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 support for coursier-based sbt launcher (WIP) #245

Closed
wants to merge 9 commits into from

Conversation

alexarchambault
Copy link

Enabled if the environment variable SBTX_COURSIER is set to "true".

I'll look into adding some tests if those changes seems ok to you, @dwijnand (and CI is fine).

Enabled if the environment variable SBTX_COURSIER is set to "true".
Copy link
Owner

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Mostly looking good, but I want to understand -Dcoursier.sbt-launcher.parse-args=true better first.

And definitely need some tests to verify (and enforce) the desired behaviour.

Thanks for picking this back up, Alex!

sbt Outdated Show resolved Hide resolved
sbt Outdated Show resolved Hide resolved
sbt Outdated
@@ -534,11 +577,19 @@ fi
# traceLevel is 0.12+
[[ -n "$trace_level" ]] && setTraceLevel

# options before a -- may be interpreted as options for itself by the
Copy link
Owner

Choose a reason for hiding this comment

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

The "for itself by the coursier-based launcher" is confusing.

And I kind of don't understand how this works. What is -Dcoursier.sbt-launcher.parse-args=true and why is it needed and not the default?

If I understand this properly it means:

  • if I specify SBTX_COURSIER=true (which sets coursier_launcher_version to true)
  • and also specify a coursier argument, let's say -CshortCircuitSbtMain=true
  • then the script will run:
java \
  -Dcoursier.sbt-launcher.parse-args=true \
  -jar ~/.sbt/launchers/coursier_1.2.4/sbt-launch.jar \
  -CshortCircuitSbtMain=true \
  --

Is that right?

Copy link
Author

@alexarchambault alexarchambault Jul 9, 2019

Choose a reason for hiding this comment

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

Just changed that to

[[ -z "$coursier_launcher_version" ]] || {
  addJava "-Dcoursier.sbt-launcher.parse-args=true"
  addCoursier "--"
}

which should be a bit clearer.

The coursier.sbt-launcher.parse-args Java property tells the launcher to parse itself arguments before a --, rather than passing everything to sbt (we handle the -C… args to it this way).

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, and why is it opt-in instead of the default behaviour?

And why is the -- required?

Couldn't csbt just always handle -C options (removing them from being passed to xMain) and handle a user-input --? I'm thinking perhaps sbt-extras shouldn't add the -- itself.

Copy link
Author

@alexarchambault alexarchambault Jul 9, 2019

Choose a reason for hiding this comment

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

That's a good point. I guess the launcher could handle the -C options itself, yes. I'm going to look into that.

Initially, coursier.sbt-launcher.parse-args was added in the context of coursier/sbt-launcher#26 (so that it doesn't try to parse arguments if not asked to do so, and IntelliJ calling it with --addPluginSbtFile … works fine).

Copy link
Owner

Choose a reason for hiding this comment

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

I think the launcher should handle arg parsing like sbt-extras does: using the equivalent of a residual_args that it forwards on. So when it sees a --addPluginSbtFile it doesn't understand it saves it for forwarding, then moves to the next argument (the argument to --addPluginSbtFile) which it doesn't understand and saves that one too.

As a safeguard, it could handle a -- as an instruction to pass the remaining arguments upstream (it doesn't pass the -- upstream). (Side-thought: maybe it should also handle -C-- also/instead? 😕)

sbt Outdated Show resolved Hide resolved
Copy link
Owner

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Bits and pieces. It's looking better and better for sbt-extras.

We should start thinking about tests for this.

sbt Outdated Show resolved Hide resolved
sbt Outdated Show resolved Hide resolved
sbt Outdated Show resolved Hide resolved
alexarchambault and others added 3 commits July 11, 2019 20:01
@dwijnand
Copy link
Owner

Hey @alexarchambault when you get a chance could you comment on how coursier/sbt-extras and coursier/sbt-launcher has been going? I'm more and more keen to switch sbt-extras to coursier's launcher.

@alexarchambault
Copy link
Author

alexarchambault commented Oct 25, 2019

Little changed since I opened this PR. I'm using it fine 100% of the time on my machine (for all sbt projects I use, and via IntelliJ and metals too). There might still be a few issues to look at (coursier/sbt-launcher#65, maybe coursier/sbt-launcher#36 too).

I also have to take coursier/sbt-launcher#102 into account too (not as is, I'm thinking of other workarounds to have sbt re-use the test-interface classloader).

@dwijnand
Copy link
Owner

I've merged master into your coursier branch and pushed that to https://github.com/dwijnand/sbt-extras/tree/coursier, which is the sbt runner I'm using as of the last 2 days.

@sideeffffect
Copy link
Contributor

@dwijnand this doesn't work behind a caching proxy like Nexus or Artifactory, even with -sbt-launch-repo and tweaked

make_coursier_url () {
  local version="$1"
  echo "https://company.com/artifactory/maven/io/get-coursier/sbt-launcher_2.12/$version/sbt-launcher_2.12-$version.jar"
}

fails with

Exception in thread "main" java.lang.NoClassDefFoundError: scala/MatchError
	at coursier.sbtlauncher.MainApp.main(MainApp.scala)
Caused by: java.lang.ClassNotFoundException: scala.MatchError
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
	... 1 more
Process exited with code 1

@dwijnand
Copy link
Owner

dwijnand commented Nov 5, 2019

Hmm, I wonder is that a mistake in how the launcher jar was created, or something like that? @alexarchambault?

@sideeffffect
Copy link
Contributor

still failing

[addJava] arg = '-Dsbt.repository.config=/etc/sbt/repositories'
[addJava] arg = '-Dsbt.override.build.repos=true'
[addJava] arg = '-Dsbt.task.timings.on.shutdown=true'
Detected sbt version 1.3.4
Downloading sbt launcher for 1.3.4:
  From  https://company.com/artifactory/maven/io/get-coursier/sbt-launcher_2.12/1.2.18/sbt-launcher_2.12-1.2.18.jar
    To  /root/.sbt/launchers/coursier_1.2.18/sbt-launch.jar

Using default jvm options
Detected Java version: 11
# Executing command line:
java
-Xms512m
-Xss2m
-XX:MaxInlineLevel=18
-XX:+UnlockExperimentalVMOptions
-XX:+UseJVMCICompiler
-Dsbt.repository.config=/etc/sbt/repositories
-Dsbt.override.build.repos=true
-Dsbt.task.timings.on.shutdown=true
-jar
/root/.sbt/launchers/coursier_1.2.18/sbt-launch.jar
check
run

saving stty: 
Exception in thread "main" java.lang.NoClassDefFoundError: scala/MatchError
	at coursier.sbtlauncher.MainApp.main(MainApp.scala)
Caused by: java.lang.ClassNotFoundException: scala.MatchError
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
	... 1 more

@dwijnand
Copy link
Owner

Btw, I have a branch https://github.com/dwijnand/sbt-extras/tree/coursier where I integrate the changes in master here with the changes in Alex's sbt-extras.

@dwijnand dwijnand closed this Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants