Skip to content
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

Merged
merged 1 commit into from Oct 27, 2020

Conversation

@JosephMoniz
Copy link
Contributor

@JosephMoniz JosephMoniz commented Oct 9, 2020

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 and scalafix 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:

repos:
  - repo: https://github.com/JosephMoniz/scalafmt
    rev: pre-commit
    hooks:
      - id: scalafmt

Usage works as expected:

➜ scalafmt-test git:(master) ✗ git add .
➜ scalafmt-test git:(master) ✗ git commit -m "test pre-commit"
scalafmt.................................................................Failed
- hook id: scalafmt
- files were modified by this hook

Reformatting...

➜ scalafmt-test git:(master) ✗ git add .
➜ scalafmt-test git:(master) ✗ git commit -m "test pre-commit"
scalafmt.................................................................Passed
Copy link
Member

@asottile asottile left a comment

looks like a good start! will probably need some tests demoing this but other than that looks great

'install',
'--default-channels=false',
'--channel=.pre-commit-channel',
'pre-commit-hook',

This comment has been minimized.

@asottile

asottile Oct 9, 2020
Member

(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

This comment has been minimized.

@JosephMoniz

JosephMoniz Oct 9, 2020
Author Contributor

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.

color: bool,
) -> Tuple[int, bytes]:
with in_env(hook.prefix):
return helpers.run_xargs(hook, ('pre-commit-hook',), file_args, color=color)

This comment has been minimized.

@asottile

asottile Oct 9, 2020
Member

I think the hardcoded string 'pre-commit-hook' needs to be used with the normal hook.entry / hook.cmd stuff? not sure

This comment has been minimized.

@JosephMoniz

JosephMoniz Oct 9, 2020
Author Contributor

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.

This comment has been minimized.

@asottile

asottile Oct 9, 2020
Member

ah yeah, the docker integration still utilizes entry in case users wanted to set arguments there:

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

This comment has been minimized.

@JosephMoniz

JosephMoniz Oct 9, 2020
Author Contributor

Hmmm, looks like entry does have meaning for the docker integration. Let me think this through a bit :)

),
)
for app_descriptor in os.listdir(channel):
app = Path(app_descriptor).stem

This comment has been minimized.

@asottile

asottile Oct 9, 2020
Member

_, app = os.path.split(app_descriptor)

This comment has been minimized.

@JosephMoniz

JosephMoniz Oct 10, 2020
Author Contributor

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?

This comment has been minimized.

@asottile

asottile Oct 10, 2020
Member

os.path.split + os.path.splitext

This comment has been minimized.

@JosephMoniz

JosephMoniz Oct 10, 2020
Author Contributor

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.

# This is a script used in CI to install coursier
set -euxo pipefail

. /etc/lsb-release

This comment has been minimized.

@asottile

asottile Oct 9, 2020
Member

probably don't need this if it's not being used, can remove this

rm -f "$ARTIFACT"
curl --location --silent --output "$ARTIFACT" "$COURSIER_URL"
check
fi

This comment has been minimized.

@asottile

asottile Oct 9, 2020
Member

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}',

This comment has been minimized.

@asottile

asottile Oct 9, 2020
Member

the channel part seems odd, is this something the target repository would need to configure directly?

This comment has been minimized.

@JosephMoniz

JosephMoniz Oct 10, 2020
Author Contributor

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.

This comment has been minimized.

@asottile

asottile Oct 10, 2020
Member

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?

This comment has been minimized.

@JosephMoniz

JosephMoniz Oct 10, 2020
Author Contributor

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.

This comment has been minimized.

@JosephMoniz

JosephMoniz Oct 10, 2020
Author Contributor

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=>
@JosephMoniz JosephMoniz requested a review from asottile Oct 12, 2020
echo "$COURSIER_HASH $ARTIFACT" | sha256sum --check
}

ARTIFACT="$HOME/.coursier/cs"

This comment has been minimized.

@asottile

asottile Oct 12, 2020
Member

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,

This comment has been minimized.

@asottile

asottile Oct 12, 2020
Member

does coursier work on windows?

This comment has been minimized.

@asottile

asottile Oct 17, 2020
Member

just making sure you saw this :)

This comment has been minimized.

@JosephMoniz

JosephMoniz Oct 18, 2020
Author Contributor

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 :)

@JosephMoniz JosephMoniz requested a review from asottile Oct 26, 2020
Copy link
Member

@asottile asottile left a comment

@asottile asottile force-pushed the JosephMoniz:add-jvm-coursier-support branch from d07bbb3 to 70ab1c3 Oct 27, 2020
@asottile asottile merged commit 9d0ab3b into pre-commit:master Oct 27, 2020
2 checks passed
2 checks passed
pre-commit.ci - pr checks completed successfully
Details
pre-commit.pre-commit #20201027.1 succeeded
Details
@asottile
Copy link
Member

@asottile asottile commented Oct 27, 2020

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

@JosephMoniz
Copy link
Contributor Author

@JosephMoniz JosephMoniz commented Oct 28, 2020

Thanks for the merge @asottile ! I can go ahead and try to put together a docs PR this Friday.

@JosephMoniz JosephMoniz deleted the JosephMoniz:add-jvm-coursier-support branch Oct 28, 2020
@asottile
Copy link
Member

@asottile asottile commented Oct 29, 2020

I left a little placeholder section for you in pre-commit/pre-commit.com#407

@asottile
Copy link
Member

@asottile asottile commented Nov 4, 2020

just a heads up, this is being asked about on SO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.