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
add coursier (jvm) as a language #1633
add coursier (jvm) as a language #1633
Conversation
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.
looks like a good start! will probably need some tests demoing this but other than that looks great
pre_commit/languages/coursier.py
Outdated
'install', | ||
'--default-channels=false', | ||
'--channel=.pre-commit-channel', | ||
'pre-commit-hook', |
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'm unfamiliar with how the tool works so I'm asking more!)
does this require the entrypoint to always be pre-commit-hook
? this might break the expectations with entry
in the pre-commit configuration
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.
Yeah, this is a bit of a weird thing. The easiest way to think about coursier in this context is that it's a bit like docker in the sense that it's fetching runnable artifacts from remote repositories (jars instead of containers), storing them locally (in a folder instead of in your local docker image registry) and then running them at a later time.
It looks like the docker integration is using the prefix
as the unique tag as a way around the fact that the local docker registry is kind of shared system wide. In this case though coursier is placing the artifacts in these namespaced/prefixed pre-commit environments so a predictable artifact location is all we need.
pre_commit/languages/coursier.py
Outdated
color: bool, | ||
) -> Tuple[int, bytes]: | ||
with in_env(hook.prefix): | ||
return helpers.run_xargs(hook, ('pre-commit-hook',), file_args, color=color) |
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 think the hardcoded string 'pre-commit-hook'
needs to be used with the normal hook.entry
/ hook.cmd
stuff? not sure
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 is very similar to the docker integration and I don't think hook.entry
or hook.cmd
have meaning there.
I'll add an example repo under testing/
to clarify and iron this out.
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.
ah yeah, the docker integration still utilizes entry
in case users wanted to set arguments there:
pre-commit/pre_commit/languages/docker.py
Lines 90 to 93 in 202f0bb
hook_cmd = hook.cmd | |
entry_exe, cmd_rest = hook.cmd[0], hook_cmd[1:] | |
entry_tag = ('--entrypoint', entry_exe, docker_tag(hook.prefix)) |
can probably do the same as that but replace the first segment with the hardcoded "container" name
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.
Hmmm, looks like entry does have meaning for the docker integration. Let me think this through a bit :)
pre_commit/languages/coursier.py
Outdated
), | ||
) | ||
for app_descriptor in os.listdir(channel): | ||
app = Path(app_descriptor).stem |
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.
_, app = os.path.split(app_descriptor)
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 also want to trim the file extension because thats what coursier expects. This doesn't appear to do that.
>>> import os
>>> _, app = os.path.split('/Users/jmoniz/src/scala-hooks/.precommit-channel/scalafmt.json')
>>> assert app == 'scalafmt'
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
The existing code does what it needs to:
>>> from pathlib import Path
>>> app = Path('/Users/jmoniz/src/scala-hooks/.precommit-channel/scalafmt.json').stem
>>> assert app == 'scalafmt'
Is there a simpler way to get the file base name?
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.
os.path.split + os.path.splitext
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.
Nice! the following works perfectly:
>>> import os
>>> path = '/Users/jmoniz/src/scala-hooks/.precommit-channel/scalafmt.json'
>>> _, app_file = os.path.split(path)
>>> app, _ = os.path.splitext(app_file)
>>> assert app == 'scalafmt'
>>>
I'll go ahead and make this change.
testing/get-coursier.sh
Outdated
# This is a script used in CI to install coursier | ||
set -euxo pipefail | ||
|
||
. /etc/lsb-release |
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.
probably don't need this if it's not being used, can remove this
testing/get-coursier.sh
Outdated
rm -f "$ARTIFACT" | ||
curl --location --silent --output "$ARTIFACT" "$COURSIER_URL" | ||
check | ||
fi |
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.
ah, this is old caching code, can probably be removed from both locations (it was only briefly used for the swift stuff but then it made the pipeline slower rather than faster)
'cs', | ||
'install', | ||
'--default-channels=false', | ||
f'--channel={channel}', |
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 channel part seems odd, is this something the target repository would need to configure directly?
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 is kind of involved and interesting so let me break this down a bit.
The json files found in the channel folder are what coursier calls app descriptors. An app descriptor is a json configuration file that describes how to pull in artifacts from maven repositories, which maven repositories to pull these artifacts from, java/scala/graal compiler options to build these artifacts with and what compiler to use to produce the executable artifacts. In a sense, these app descriptors are to their built on disk artifacts what docker files are to their built docker images.
Coursier channels are basically pointers to repositories of app descriptors. There exist a default channel but it contains fairly plain app descriptors. If you want to do something more fancy like compile scalafmt
to a native image binary you're going to have to create a custom channel somewhere (gh repo, on disk folder or a web hosted json file), add your custom app descriptor there and point coursier at that channel.
This coursier integration assumes each repo a coursier hook lives in, to have a repo local channel folder specifically for pre-commit. It then attempts to build + locally install all the apps defined in the repo local pre-commit channel. Then the hook repo can export multiple pre-commit hooks pointing at different locally built coursier binaries with varying default arguments.
Similar how there is a docker + docker-image integration, I can see this being fast followed with an additional coursier integration that just points at remote hosted channels (other gh repos or web hosted json files) and imports artifacts from those.
The goal of this integration is to build executables from locally specified app descriptors though.
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 guess my concern with having a custom channel is that would make it less likely for upstream source repositories to add support for pre-commit (as they'd have to not only maintain a configuration file but also some build scripts) -- thoughts?
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.
Ah, yeah I see your concern. Thinking through this a bit, i think this concern can be addressed by adding additional languages/integrations specific to JVM build tools later on. For example, if we were to add additional integrations for SBT and Mill we would be able to build these artifacts directly from their already existing build files. For other JVM languages and ecosystems additional integrations would have to be built, IE: maven + gradle for java.
The usage for this integration I see as more like something a user would use to create a docker based commit hook instead of in an upstream repo via the projects specific build tool. It's a nice kind of gap filler solution for more refined project/language native solutions later.
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 motivating example for thinking of this as a docker like integration vs a language/project native integration is the following demos of using coursier to luanch repls in different languages.
Jython repl:
> cs launch org.python:jython-standalone:2.7.0 -M org.python.util.jython
https://repo1.maven.org/maven2/org/python/jython-standalone/2.7.0/jython-standa…
100.0% [##########] 1.4 KiB (2.0 KiB / s)
https://repo1.maven.org/maven2/org/python/jython-standalone/2.7.0/jython-standa…
100.0% [##########] 35.3 MiB (10.2 MiB / s)
Jython 2.7.0 (default:9987c746f838, Apr 29 2015, 02:25:11)
[OpenJDK 64-Bit Server VM (AdoptOpenJDK)] on java1.8.0_262
Type "help", "copyright", "credits" or "license" for more information.
>>>
scala repl (ammonite):
> cs launch com.lihaoyi:ammonite_2.12.8:1.6.0 -M ammonite.Main
https://repo1.maven.org/maven2/com/lihaoyi/ammonite_2.12.8/1.6.0/ammonite_2.12.…
100.0% [##########] 2.4 KiB (7.0 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-ops_2.12/1.6.0/ammonite-ops…
100.0% [##########] 1.3 KiB (9.1 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-terminal_2.12/1.6.0/ammonit…
100.0% [##########] 1.5 KiB (9.0 KiB / s)
https://repo1.maven.org/maven2/com/github/scopt/scopt_2.12/3.5.0/scopt_2.12-3.5…
100.0% [##########] 1.6 KiB (21.9 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-repl_2.12.8/1.6.0/ammonite-…
100.0% [##########] 2.9 KiB (14.1 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-interp_2.12.8/1.6.0/ammonit…
100.0% [##########] 2.4 KiB (10.5 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-runtime_2.12/1.6.0/ammonite…
100.0% [##########] 2.1 KiB (9.1 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-util_2.12/1.6.0/ammonite-ut…
100.0% [##########] 1.9 KiB (8.2 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/fansi_2.12/0.2.4/fansi_2.12-0.2.4.pom
100.0% [##########] 1.8 KiB (21.6 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/os-lib_2.12/0.2.6/os-lib_2.12-0.2.6.…
100.0% [##########] 1.2 KiB (14.3 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/scalaparse_2.12/2.1.0/scalaparse_2.1…
100.0% [##########] 1.2 KiB (14.8 KiB / s)
https://repo1.maven.org/maven2/io/get-coursier/coursier-cache_2.12/1.1.0-M7/cou…
100.0% [##########] 1.7 KiB (20.8 KiB / s)
https://repo1.maven.org/maven2/org/scalaj/scalaj-http_2.12/2.4.0/scalaj-http_2.…
100.0% [##########] 2.7 KiB (31.6 KiB / s)
https://repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.0/scala-librar…
100.0% [##########] 1.5 KiB (12.6 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/pprint_2.12/0.5.2/pprint_2.12-0.5.2.…
100.0% [##########] 2.6 KiB (34.8 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/upickle_2.12/0.7.1/upickle_2.12-0.7.…
100.0% [##########] 1.6 KiB (20.6 KiB / s)
https://repo1.maven.org/maven2/io/get-coursier/coursier_2.12/1.1.0-M7/coursier_…
100.0% [##########] 1.7 KiB (22.8 KiB / s)
https://repo1.maven.org/maven2/net/java/dev/jna/jna/4.2.2/jna-4.2.2.pom
100.0% [##########] 1.3 KiB (29.8 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/fastparse_2.12/2.1.0/fastparse_2.12-…
100.0% [##########] 1.2 KiB (14.8 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ujson_2.12/0.7.1/ujson_2.12-0.7.1.pom
100.0% [##########] 1.2 KiB (14.6 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/upack_2.12/0.7.1/upack_2.12-0.7.1.pom
100.0% [##########] 1.2 KiB (14.6 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/upickle-implicits_2.12/0.7.1/upickle…
100.0% [##########] 1.8 KiB (21.1 KiB / s)
https://repo1.maven.org/maven2/org/scala-lang/modules/scala-xml_2.12/1.1.0/scal…
100.0% [##########] 2.7 KiB (56.0 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/geny_2.12/0.1.5/geny_2.12-0.1.5.pom
100.0% [##########] 1.5 KiB (9.2 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/acyclic_2.12/0.1.5/acyclic_2.12-0.1.…
100.0% [##########] 2.0 KiB (28.2 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/upickle-core_2.12/0.7.1/upickle-core…
100.0% [##########] 1.1 KiB (15.0 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/utest_2.12/0.6.4/utest_2.12-0.6.4.pom
100.0% [##########] 2.3 KiB (32.0 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/sourcecode_2.12/0.1.5/sourcecode_2.1…
100.0% [##########] 2.0 KiB (18.5 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/upickle-core_2.12/0.7.1/upickle-core…
100.0% [##########] 106.4 KiB (695.5 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/os-lib_2.12/0.2.6/os-lib_2.12-0.2.6.…
100.0% [##########] 317.7 KiB (1.4 MiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/scalaparse_2.12/2.1.0/scalaparse_2.1…
100.0% [##########] 431.4 KiB (2.0 MiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-util_2.12/1.6.0/ammonite-ut…
100.0% [##########] 173.7 KiB (443.2 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-terminal_2.12/1.6.0/ammonit…
100.0% [##########] 171.6 KiB (866.6 KiB / s)
https://repo1.maven.org/maven2/net/java/dev/jna/jna/4.2.2/jna-4.2.2.jar
100.0% [##########] 1.1 MiB (3.8 MiB / s)
https://repo1.maven.org/maven2/org/scala-lang/modules/scala-xml_2.12/1.1.0/scal…
100.0% [##########] 535.7 KiB (1.4 MiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/fastparse_2.12/2.1.0/fastparse_2.12-…
100.0% [##########] 297.6 KiB (1.3 MiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/pprint_2.12/0.5.2/pprint_2.12-0.5.2.…
100.0% [##########] 147.0 KiB (1.4 MiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/upickle-implicits_2.12/0.7.1/upickle…
100.0% [##########] 171.6 KiB (783.7 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/sourcecode_2.12/0.1.5/sourcecode_2.1…
100.0% [##########] 108.9 KiB (1.0 MiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-ops_2.12/1.6.0/ammonite-ops…
100.0% [##########] 122.5 KiB (554.2 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/fansi_2.12/0.2.4/fansi_2.12-0.2.4.jar
100.0% [##########] 62.8 KiB (615.6 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-runtime_2.12/1.6.0/ammonite…
100.0% [##########] 217.8 KiB (738.3 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/utest_2.12/0.6.4/utest_2.12-0.6.4.jar
100.0% [##########] 306.8 KiB (1.1 MiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/upickle_2.12/0.7.1/upickle_2.12-0.7.…
100.0% [##########] 90.7 KiB (1.0 MiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ujson_2.12/0.7.1/ujson_2.12-0.7.1.jar
100.0% [##########] 193.4 KiB (1.4 MiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/geny_2.12/0.1.5/geny_2.12-0.1.5.jar
100.0% [##########] 54.7 KiB (692.8 KiB / s)
https://repo1.maven.org/maven2/com/github/scopt/scopt_2.12/3.5.0/scopt_2.12-3.5…
100.0% [##########] 71.8 KiB (944.6 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/upack_2.12/0.7.1/upack_2.12-0.7.1.jar
100.0% [##########] 90.5 KiB (878.7 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/acyclic_2.12/0.1.5/acyclic_2.12-0.1.…
100.0% [##########] 56.9 KiB (824.5 KiB / s)
https://repo1.maven.org/maven2/org/scalaj/scalaj-http_2.12/2.4.0/scalaj-http_2.…
100.0% [##########] 124.9 KiB (690.1 KiB / s)
https://repo1.maven.org/maven2/io/get-coursier/coursier_2.12/1.1.0-M7/coursier_…
100.0% [##########] 1.4 MiB (1.5 MiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite-repl_2.12.8/1.6.0/ammonite-…
100.0% [##########] 177.6 KiB (569.1 KiB / s)
https://repo1.maven.org/maven2/io/get-coursier/coursier-cache_2.12/1.1.0-M7/cou…
100.0% [##########] 178.3 KiB (670.4 KiB / s)
https://repo1.maven.org/maven2/com/lihaoyi/ammonite_2.12.8/1.6.0/ammonite_2.12.…
100.0% [##########] 150.6 KiB (466.4 KiB / s)
Loading...
Compiling (synthetic)/ammonite/predef/interpBridge.sc
Compiling (synthetic)/ammonite/predef/replBridge.sc
Compiling (synthetic)/ammonite/predef/DefaultPredef.sc
Welcome to the Ammonite Repl 1.6.0
(Scala 2.12.8 Java 1.8.0_262)
If you like Ammonite, please support our development at www.patreon.com/lihaoyi
@
Clojure:
cs launch org.clojure:clojure:1.7.0 -M clojure.main
https://repo1.maven.org/maven2/org/clojure/clojure/1.7.0/clojure-1.7.0.jar
100.0% [##########] 3.7 MiB (5.1 MiB / s)
Clojure 1.7.0
user=>
testing/get-coursier.sh
Outdated
echo "$COURSIER_HASH $ARTIFACT" | sha256sum --check | ||
} | ||
|
||
ARTIFACT="$HOME/.coursier/cs" |
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 only wrote to the homedir before because GA couldn't cache elsewhere -- it should be in /tmp
instead
@@ -40,6 +40,10 @@ def cmd_output_mocked_pre_commit_home( | |||
return ret, out.replace('\r\n', '\n'), None | |||
|
|||
|
|||
skipif_cant_run_coursier = pytest.mark.skipif( | |||
os.name == 'nt' or parse_shebang.find_executable('cs') is None, |
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.
does coursier work on windows?
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.
just making sure you saw this :)
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.
Thanks for the ping :)
According to the docs Coursier should work on windows. I don't have access to windows or know much about scripting + installation on windows but I can take a pass at getting this done soon-is.
This is my current 10% time project so I won't be able to poke around with it until next week :)
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.
thanks again for the awesome work on this! this will be in the next 2.8.0 release (I need to fix a few things for nodejs and ruby before that's ready to go out the door) would it be too much to ask for a docs pr to https://github.com/pre-commit/pre-commit.com (similar to pre-commit/pre-commit.com#397) documenting the new language? if not I can handle it |
Thanks for the merge @asottile ! I can go ahead and try to put together a docs PR this Friday. |
I left a little placeholder section for you in pre-commit/pre-commit.com#407 |
just a heads up, this is being asked about on SO |
This change adds coursier as a "language" to pre-commit with the goal of allowing the JVM ecosystem tooling to be used in pre-commit hooks.
coursier is a tool for fetching, building and launching JVM applications directly from the command line. It also has some features for easily compiling these applications to native binaries via GraalVM so you don't have to pay the JVM startup cost for invoking CLI tooling.
The big motivation for me making this change was to be able to use
scalafmt
andscalafix
via pre-commit. I created a mirror hook-repo of scalafmt in order to test this change out.This repo is usable via the following pre-commit config:
Usage works as expected: