Skip to content

Fix passing jvm options #14073

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 13, 2021
Merged

Fix passing jvm options #14073

merged 3 commits into from
Dec 13, 2021

Conversation

BarkingBad
Copy link
Contributor

No description provided.

@BarkingBad BarkingBad requested a review from philwalk December 8, 2021 18:21
@BarkingBad BarkingBad linked an issue Dec 8, 2021 that may be closed by this pull request
Cannot pass system properties in comand line #14005
Closed
@BarkingBad
Copy link
Contributor Author

@philwalk I wonder whether we correctly interpret the -J options. Actually, they have to be preprocessed earlier because we cannot inject them into JVM. The question is, whether we should check them at all in MainGenericRunner then (now we do so). I am also thinking about -D option. I feel like making "syntactic sugar" for passing -D options could be viable, yet the proper way to inject environmental variables should be passing -J-D... because they are a subset of JVM options. Is there any spec on that? WDYT?

@BarkingBad
Copy link
Contributor Author

Also passing env variable to scala3 via coursier is working, because coursier runs the JVM with given java setting, the problem is with our bash scala runner only

@BarkingBad BarkingBad force-pushed the passing-jvm-opts-to-scala branch from 0f3c819 to 77b4cba Compare December 8, 2021 18:28
@smarter
Copy link
Member

smarter commented Dec 8, 2021

We should support -D and not just -J-D since the scala 2 shell scripts support -D

smarter
smarter reviewed Dec 8, 2021
View reviewed changes
@BarkingBad BarkingBad force-pushed the passing-jvm-opts-to-scala branch from 77b4cba to 61a3c08 Compare December 9, 2021 13:13
@BarkingBad BarkingBad marked this pull request as ready for review December 9, 2021 13:13
@BarkingBad BarkingBad force-pushed the passing-jvm-opts-to-scala branch from 61a3c08 to c5ef535 Compare December 9, 2021 13:18
@BarkingBad
Copy link
Contributor Author

BarkingBad commented Dec 9, 2021 •
edited
Loading

@smarter, do you think that we could backport this?

@smarter
Copy link
Member

smarter commented Dec 9, 2021

Don't think it's critical enough.

@BarkingBad
Copy link
Contributor Author

These guys have problem because of the issue

Thanks for the workaround michelou. REPL was an example. We have the same problem running a scala executable (.jar). This bug forbids us to deploy the scala suite (ias) in production.

#14005 (comment)

@BarkingBad BarkingBad force-pushed the passing-jvm-opts-to-scala branch from c5ef535 to 79d9f5f Compare December 9, 2021 14:56
@smarter
Copy link
Member

smarter commented Dec 9, 2021

I'm not opposed to a backport but you'll have to ask the release manager for this release (@Kordyjan ?) to see if it can be done in time.

@Kordyjan
Copy link
Contributor

Kordyjan commented Dec 9, 2021

As we are still waiting for #13777, I think we can give it a try. I think if we manage to merge this PR tomorrow and have backporting PR merged no later than Monday noon we can squeeze this fix into 3.1.1-RC2.

@BarkingBad
Copy link
Contributor Author

Well, yes, but actually no.
The problem is we had these tests failing for a long long time (see https://github.com/lampepfl/dotty/runs/4470356079?check_suite_focus=true#step:9:764) - the SCALA_HOME is empty and every test was failing silently

I enabled them by adding dist/pack. Then it appeared some of them are failing. The problem is that the shebang on linux cannot take parameters. I fixed it using this trick: https://unix.stackexchange.com/a/477651

The third problem was with handling our SCALA_OPTS property. For empty value we were passing empty string, which was breaking the compilation.

Now it should work as expected

@BarkingBad
Copy link
Contributor Author

I also had to remove the ReplTests which cannot work - we are using mock driver in the same JVM context, thus we cannot just pass the -D option, because it won't be set up anywhere

@BarkingBad
Copy link
Contributor Author

It is working from coreutils 8.30 according to stack answer. For me, it was working locally as expected. Nonetheless, it's not our problem, but Linux, so if one's environment doesn't support multiple arguments in a shebang using this trick, he cannot benefit from it either way.

BarkingBad
BarkingBad commented Dec 9, 2021
View reviewed changes
@BarkingBad
Copy link
Contributor Author

@philwalk now since I "enabled" scripting tests do you mind adding some assertions when the scripts fail? Many of them have the if checking if the process was successful, but there is no else branch to throw some kind of assertion error to raise the problem, for example here we just skip the validTest == false, or is it made intentionally?

@BarkingBad
Copy link
Contributor Author

The problem is we had these tests failing for a long long time

Do you have a link to a discussion of this issue? The link you provided currently shows passing tests.

The test is passing, but it prints stderr with non zero return codes

@BarkingBad
Copy link
Contributor Author

I'm away from home for the weekend, but as I looked yesterday on logs there was some stderr pointing to different java versions IIRC, yet I had no time to fix it (it's CI subject problem, not the scala logic)

@BarkingBad
Copy link
Contributor Author

@philwalk can you push directly on top of my branch? I think it is possible for maintainers. If not, maybe you could do some branch with fix and I'll cherry-pick your commit, would that be OK?

Create BashScriptsTests.scala
2d70b59 
This sets `javaHome` at the head of the PATH for `bashCommands`.
@BarkingBad BarkingBad merged commit dcb123d into scala:master Dec 13, 2021
@BarkingBad
Copy link
Contributor Author

@Kordyjan should we backport these changes?

@BarkingBad
Copy link
Contributor Author

@philwalk apparently this PR broke CI on master: the logs are:

Logs 

 cwd: /__w/dotty/dotty
classpath: /__w/dotty/dotty:/__w/dotty/dotty/dist/target/pack/lib/scala-library-2.13.6.jar:/__w/dotty/dotty/dist/target/pack/lib/scala3-library_3-3.1.2-RC1-bin-SNAPSHOT.jar:/__w/dotty/dotty/dist/target/pack/lib/scala-asm-9.1.0-scala-1.jar:/__w/dotty/dotty/dist/target/pack/lib/compiler-interface-1.3.5.jar:/__w/dotty/dotty/dist/target/pack/lib/scala3-interfaces-3.1.2-RC1-bin-SNAPSHOT.jar:/__w/dotty/dotty/dist/target/pack/lib/scala3-compiler_3-3.1.2-RC1-bin-SNAPSHOT.jar:/__w/dotty/dotty/dist/target/pack/lib/tasty-core_3-3.1.2-RC1-bin-SNAPSHOT.jar:/__w/dotty/dotty/dist/target/pack/lib/scala3-staging_3-3.1.2-RC1-bin-SNAPSHOT.jar:/__w/dotty/dotty/dist/target/pack/lib/scala3-tasty-inspector_3-3.1.2-RC1-bin-SNAPSHOT.jar:/__w/dotty/dotty/dist/target/pack/lib/jline-reader-3.19.0.jar:/__w/dotty/dotty/dist/target/pack/lib/jline-terminal-3.19.0.jar:/__w/dotty/dotty/dist/target/pack/lib/jline-terminal-jna-3.19.0.jar:/__w/dotty/dotty/dist/target/pack/lib/jna-5.3.1.jar:dist/target/pack/lib/protobuf-java-3.7.0.jar:dist/target/pack/lib/flexmark-ext-emoji-0.42.12.jar:dist/target/pack/lib/compiler-interface-1.3.5.jar:dist/target/pack/lib/flexmark-ext-anchorlink-0.42.12.jar:dist/target/pack/lib/scala3-staging_3-3.1.2-RC1-bin-SNAPSHOT.jar:dist/target/pack/lib/jna-5.3.1.jar:dist/target/pack/lib/liqp-0.6.8.jar:dist/target/pack/lib/snakeyaml-1.23.jar:dist/target/pack/lib/ST4-4.0.7.jar:dist/target/pack/lib/flexmark-ext-autolink-0.42.12.jar:dist/target/pack/lib/dist_3-3.1.2-RC1-bin-SNAPSHOT.jar:dist/target/pack/lib/flexmark-0.42.12.jar:dist/target/pack/lib/jsoup-1.13.1.jar:dist/target/pack/lib/antlr-3.5.1.jar:dist/target/pack/lib/util-interface-1.3.0.jar:dist/target/pack/lib/flexmark-formatter-0.42.12.jar:dist/target/pack/lib/flexmark-jira-converter-0.42.12.jar:dist/target/pack/lib/scala3-library_3-3.1.2-RC1-bin-SNAPSHOT.jar:dist/target/pack/lib/antlr-runtime-3.5.1.jar:dist/target/pack/lib/flexmark-ext-ins-0.42.12.jar:dist/target/pack/lib/flexmark-ext-gfm-tables-0.42.12.jar:dist/target/pack/lib/autolink-0.6.0.jar:dist/target/pack/lib/jackson-dataformat-yaml-2.9.8.jar:dist/target/pack/lib/scala3-tasty-inspector_3-3.1.2-RC1-bin-SNAPSHOT.jar:dist/target/pack/lib/flexmark-html-parser-0.42.12.jar:dist/target/pack/lib/jline-reader-3.19.0.jar:dist/target/pack/lib/scala3-interfaces-3.1.2-RC1-bin-SNAPSHOT.jar:dist/target/pack/lib/jline-terminal-3.19.0.jar:dist/target/pack/lib/flexmark-util-0.42.12.jar:dist/target/pack/lib/flexmark-ext-gfm-strikethrough-0.42.12.jar:dist/target/pack/lib/jackson-databind-2.2.3.jar:dist/target/pack/lib/tasty-core_3-3.1.2-RC1-bin-SNAPSHOT.jar:dist/target/pack/lib/jackson-core-2.9.8.jar:dist/target/pack/lib/flexmark-ext-wikilink-0.42.12.jar:dist/target/pack/lib/flexmark-ext-superscript-0.42.12.jar:dist/target/pack/lib/scaladoc_3-3.1.2-RC1-bin-SNAPSHOT.jar:dist/target/pack/lib/flexmark-ext-gfm-tasklist-0.42.12.jar:dist/target/pack/lib/scala-asm-9.1.0-scala-1.jar:dist/target/pack/lib/scala-library-2.13.6.jar:dist/target/pack/lib/flexmark-ext-tables-0.42.12.jar:dist/target/pack/lib/flexmark-ext-yaml-front-matter-0.42.12.jar:dist/target/pack/lib/scala3-compiler_3-3.1.2-RC1-bin-SNAPSHOT.jar:dist/target/pack/lib/jline-terminal-jna-3.19.0.jar:dist/target/pack/lib/jackson-annotations-2.2.3.jar
output  []
expected[/__w/dotty/dotty]
osname[linux]
Error:  Test dotty.tools.scripting.BashScriptsTests.verifyScalaOpts failed: java.lang.AssertionError: assertion failed: script /__w/dotty/dotty/compiler/target/scala-3.1.1-RC1/test-classes/scripting/classpathReport.sc did not report valid java.class.path first entry, took 55.464 sec

This is strange, becuase there is cwd in logs. Do you have any idea why it failed on non-bootstrapped tests?

@BarkingBad
Copy link
Contributor Author

Also, test_windows_full failed because of missing artifacts (it didn't come up in PR because there is only fast check), this PR should fix it, and check if test_windows_full is passing #14106

@bishabosha bishabosha mentioned this pull request Dec 14, 2021
Closed
@BarkingBad BarkingBad mentioned this pull request Dec 17, 2021
Merged
@Kordyjan Kordyjan added this to the 3.1.2 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.1.2
Development

Successfully merging this pull request may close these issues.

Cannot pass system properties in comand line
3 participants
@BarkingBad @smarter @Kordyjan