FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add github.com/scop/wrun by scop · Pull Request #904 · pre-commit/pre-commit.com · GitHub

Repository navigation

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension .yaml  (1) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
1 change: 1 addition & 0 deletions all-repos.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@
- 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

scop Nov 24, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

- https://github.com/BlankSpruce/gersemi
- https://github.com/realm/SwiftLint
- https://gitlab.com/bmares/check-json5
Expand Down

Back | FazBrowse Home | New Git URL