GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add github.com/scop/wrun by scop · Pull Request #904 · pre-commit/pre-commit.com

Conversation

@scop

@scop scop commented Nov 26, 2023

Copy link
Copy Markdown
Contributor

my new repository:

  • is added to the bottom or with existing repos from the same account
  • contains a license
  • is not language: system, language: script, language: docker, or language: docker_image
  • does not contain "pre-commit" in the name

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@scop
Add github.com/scop/wrun
b4478f9
asottile
asottile reviewed Nov 27, 2023
View reviewed changes
Comment thread all-repos.yaml
- https://github.com/adamchainz/django-upgrade
- https://github.com/scop/pre-commit-shfmt
- https://github.com/scop/pre-commit-perlcritic
- https://github.com/scop/wrun

@asottile asottile Nov 27, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this downloads tools at runtime rather than at build time so it's not a good fit for the framework -- perhaps you can chime in on pre-commit/pre-commit#1453 and pre-commit/pre-commit#3020 which are trying to solve this more generically (and correctly)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@scop scop Nov 27, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrun works for its intended purposes and I think it's correct enough for what it's meant for. Of course there's room for some improvement always though. Anyway, it also works with pre-commit as it stands, the only thing I'm aware of where it does not work is pre-commit.ci. BTW pre-commit.ci's runtime network prohibition also runs at odds with other tools, such as dprint, as well as whatever would need to access the network for other reasons besides installing something (hypothetical, haven't come across anything, but easily imaginable).

Good to hear that a pre-commit solution to this problem is in the works, but I'm not really that interested in one that solves it only within pre-commit. Will drop a note about wrun to the mentioned issues in case the authors of those implementations wish to borrow some from there.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@asottile asottile Nov 27, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem is more that there's no write protection to the pre-commit cache at runtime (intentionally) and so writes in general break the cache integrity and at worst concurrent writes permanently poison the cache and so writing at runtime is unsupported

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@scop scop Nov 27, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A workaround for that could be if pre-commit.ci provided a way to cache additional paths, not just the pre-commit cache. Whatever happens in them would be the responsibility of something else besides pre-commit. Can imagine that this would likely make sharing such additional caches pretty much a no go, as well as their retention suboptimal.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@asottile asottile Nov 27, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not a pre-commit.ci issue -- it's a pre-commit issue. writes to pre-commit's cache at runtime are unsafe

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@scop scop Nov 27, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to wrun, the cache dir is naturally configurable. As it sounds unlikely that pre-commit.ci would become supported for this tool, I'll likely remove the helper option to (ab)use a subdir in pre-commit's cache. Other CI systems that I'm aware of have the ability to cache pretty much arbitrary paths, should be no problem to configure the wrun cache if one is using it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@scop scop Nov 24, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll likely remove the helper option to (ab)use a subdir in pre-commit's cache.

FTR, this option has been removed since, wrun no longer helps or recommends the user in any way to touch pre-commit's cache.

So in that sense I suppose that side of the blocker against adding this in pre-commit's supported hooks list is now gone, and the addition could be reconsidered. The build time vs runtime distinction still exists though, but that's not something wrun can affect alone. I suppose pre-commit could, if it provided more flexibility to configure what happens in each of those phases. wrun's --dry-run mode could be then used in a hypothetical "build" stage to do the downloads

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@asottile asottile closed this Nov 27, 2023
@scop scop mentioned this pull request Nov 27, 2023
Create a 'download' language for fetching pre-built binaries pre-commit/pre-commit#1453
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asottile asottile asottile left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@scop @asottile
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.