-
-
Notifications
You must be signed in to change notification settings - Fork 767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create a 'download' language for fetching pre-built binaries #1453
Comments
I don't think that would be a feature to also for shellcheck in particular, shellcheck-py is the suggested tool for pre-commit note that a url alone does not help you here since you need to work about platform and architecture and likely checksumming, etc. for |
That's a good point, yup
Is always installing from source ideal? The language a hook is written in feels like it should be an implementation detail as much as possible. We don't use Go at all for instance, but if we want to use a hook written in Go, using Appreciate the need to handle platforms, architectures and checksums, but being able to point at a pre-built binary, pull it and run it seems useful? |
yeah I'm not saying |
I don't think it's that simple - I don't think I could have added any more caveats up top 😄 I guess I just wanted to know if If either are true, I wasn't going to expend any energy having a deeper look at it. On the basis that it seems to get a luke warm reception, I'll have a proper look. And hey, worst case, I'll run into a blocker and leave a comment here to save the next person from wasting time on it :) |
yeah nothing has been attempted there |
would you like to design / work on such a thing? |
Yes I would like to. I'm aiming to look at it in the next day or so |
Timeline was ambitious but not forgotten about. Made a start at the time but need to find some more time to pick it up again. If you'd rather have a cleaner issues page, by all means close this and I can come back when/if there's a PR to be had |
I've been working on this further and noticed a general case which might be simpler, and cover most of what I was looking for in the first place. It's 50/50 on whether you'll hate the idea, if you don't, I'll raise a separate issue to track it and work on it. Premise 1: A lot of tools that aren't pip installable are on GitHub, and when they are released, artifacts are available from the releases page, with artifacts built for most OS + arch combinations Packaging aside (zip vs tar.xz vs bare), this is true for a number of repos - some examples: What do you think to the following rough proposal for config?:
With the benefits as before, that you don't need to have the toolchain installed for whatever language the hook was written in, and there's a good chance it can download and unpack an asset faster than it can build from source on that initial environment setup. Is that a terrible idea? |
Some quick experimentation on the above worked pretty well. Only framework change I had to make was to add
Timing pre-commit:
If you hate it @asottile I'll abandon it, else I can tidy it up and put out a PR |
it kinda doesn't gel nicely with the framework which expects the cloned repository to have all the necessary things (and would only work with tags and not frozen revs, and is a bit too github-specific). I was hoping for something a little more generic 🤔 the description of your changes wrt src / rev also concerns me -- the installed state is supposed to dictate whether something is installed or not and it sounds like parts of that are missing |
Appreciate the speedy response
I see what you mean, though tempted to argue this already isn't the case - if the user needs to have the matching runtime installed to build the source. If the maintainer has gone to the 'effort' of building binaries, it seems a shame not to use them?
Yup, that's what I started on and I thought this could be additional to a generic approach, to optimise this seemingly common scenario and greatly reduce the config needed. I can stick to just the one language addition though.
I think I worded it badly, the src/rev are just needed when installing the environment purely to build the API URL. Everything works normally wrt to state |
Before I tidy this up and figure out how to write tests for it all, could I run the (working) proposal past you for syntax: Example (checksum is optional - provided in the darwin case):
Keen to know how close this is to what you're expecting/what needs to change. Cheers! |
(copying from discord)
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
nah you're missing the point, the whole point of pre-commit is it manages your tools |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
As an update: we're rolling through the implementation of this. We've realised that we need an extra line to the additional dependencies to specify when the binary should be stored so that we have a consistent binary name for the
|
This commit implements pre-commit#1453 (comment)
Pull request generated at |
This commit implements pre-commit#1453 (comment)
Looks like there's quite a bit of movement in this area currently. Anyway I filed pre-commit/pre-commit.com#904 which wasn't accepted but was asked to chime in here so here goes: https://github.com/scop/wrun implements a generic "download launcher" with caching and archive support, not specific to pre-commit. It does work with pre-commit too though, just not with pre-commit.ci as it stands, and perhaps not fully the way pre-commit's languages are intended to work. It might contain some ideas that a pre-commit specific implementation could make use of. One comment about the subresource identity idea here: I considered that for wrun as well, but stuck with the "regular" hex digests with algorithm prefix. This is because these downloads are not really "subresources" in my opinion, and because IME hex digests are vastly more common than base64 ones in this problem space. Using base64 would mean one would practically need to generate or convert the digests separately instead of being able to use upstream provided ones as is. It might require some custom / not that common tooling as well or a few more hops, as the standard |
I just created a simple Rust hook and wondered how Anyway, a workaround I've used previously is to distribute binaries via Pip. Yes it sounds mad, but Pip is pretty much universally available and works quite well for distributing Rust programs. Here's an example - you can So, for Rust programs at least, can I:
Is there any reason that wouldn't work? Will that all be cached? |
they would never know, it is not exposed outside of the pre-commit cache. it's not installed to their homedir at all you must be confused |
Ah even better. Looks like I misread the code! I guess my suggestion would still be good from a setup speed & disk usage point of view, but I don't think I care enough about that to bother seeing as it is one-time. |
I'm aware of the requirement that additional dependencies for system hooks - the 'escape hatch' for hooks that don't fit into the python/node/go etc. managed environments list, such as shellcheck, must be installed outside of pre-commit in order to work.
Also aware of the python module that can be used to 'mirror' shellcheck in particular, though that feels a little hacky.
I'm sure you've given this thought before, and there's a very good reason why this hasn't been done, but, is it conceivable, with a PR I'd be willing to write, that additional dependencies could be supported for non-python etc. hooks?
I'm thinking along the lines of being able to specify a URL to a ZIP, which pre-commit would
wget
into its cache directory, storing a checksum so it could 'manage' when to re-download it, and then adding the directory to the front of$PATH
when running the hook.I'm sure it's not possible, but would be keen to know why, as there are a few hooks I use that this applies too and would feel like a useful addition for others.
The text was updated successfully, but these errors were encountered: