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

Runner scripts -usebootcp and require javaBootClassPath #10350

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

som-snytt
Copy link
Contributor

Follow-up to #10336 which too eagerly ignores javaBootClassPath, which is where the scala scripts put the library (via -Dscala.boot.class.path). That means the script needs -nobootcp.

It's only necessary to turn off jrt when user has set the -javabootclasspath explicitly.

The comment in PathResolver:

    // TODO It must be time for someone to figure out what all these things
    // are intended to do.  This is disabled here because it was causing all
    // the scala jars to end up on the classpath twice: one on the boot
    // classpath as set up by the runner (or regular classpath under -nobootcp)
    // and then again here.

Probably -bootclasspath was intended for "Scala platform" but is possibly generally ignored by clients? Scala 3 has the same compiler options (except -nobootcp) but I haven't looked at how they are used.

The other factor is -Dscala.usejavacp. All of these mechanisms were tagged for removal a decade ago, but it's never the right time, although springtime calls for spring cleaning.

@scala-jenkins scala-jenkins added this to the 2.13.12 milestone Mar 22, 2023
@som-snytt som-snytt modified the milestones: 2.13.12, 2.13.11 Mar 22, 2023
@som-snytt som-snytt marked this pull request as ready for review March 22, 2023 11:45
@SethTisue
Copy link
Member

Would it be practical to add test coverage?

@SethTisue SethTisue added the prio:hi high priority (used only by core team, only near release time) label Mar 22, 2023
@som-snytt
Copy link
Contributor Author

Script testing was added on dotty. Compare https://github.com/scala/scala/tree/2.13.x/test/script-tests readme.

@SethTisue SethTisue self-assigned this Mar 22, 2023
@SethTisue
Copy link
Member

Okay.. I guess we just need to be extra careful about testing this stuff manually, and summarizing what we did on the PR in question.

Merging this since Lukas also already noticed the breakage over on #10336.

@SethTisue SethTisue merged commit 0362712 into scala:2.13.x Mar 28, 2023
@SethTisue
Copy link
Member

By the time we release 2.13.11, it will have been six months since 2.13.10, with a considerable amount of change having landed. It won't be the end of the world if we end up having to do a 2.13.12 with a few adjustments.

@som-snytt
Copy link
Contributor Author

Thanks, I'm getting tired of -nobootcp in local testing, since I usually dist/mkPack, but I didn't want to complain to the waiter.

@som-snytt som-snytt deleted the followup/javabootclasspath branch March 28, 2023 16:12
@lrytz
Copy link
Member

lrytz commented Mar 29, 2023

Do you (@SethTisue or @som-snytt) understand why quick/bin broke with #10336 and how this PR fixes it? How confident are we about not introducing a regression? What does the overall change fix?

@SethTisue
Copy link
Member

SethTisue commented Mar 29, 2023

I plan to study that more in the few weeks we still have, but not this week. You're of course free, encouraged even :-), to study it as well.

Reverting the whole business until after 2.13.11 remains a possibility. I merged this PR because I didn't want to leave 2.13.x in a worst-of-both-worlds state in the meantime.

@SethTisue
Copy link
Member

What does the overall change fix?

scala/bug#12257

@som-snytt
Copy link
Contributor Author

Before I forget again, --release is not always perfect because of binary compatibility of actual rt.jar (the ticket mentions what that was, and the Javadoc says API was added in an update). So if they specify -javabootclasspath with their exact rt.jar, while building with current JVM, we don't want jrt classpath (or ct.sym) to take precedence.

My glib fix was that the classpath components are mutually exclusive: you specify either --release or -javabootclasspath. But I neglected slash blocked it out that usebootcp means javaBootClassPath typically is the scala library, where -javabootclasspath was not specified.

Overall change is not to use jrt when the user specifies -javabootclasspath.

Future change, if worth the trouble, is to clean up (remove) -usebootcp, -usejavacp, -bootclasspath. I think that is called "quiet quitting" JVM 8.

@SethTisue SethTisue removed the prio:hi high priority (used only by core team, only near release time) label May 4, 2023
@SethTisue
Copy link
Member

SethTisue commented May 25, 2023

Do you (@SethTisue or @som-snytt) understand why quick/bin broke with #10336 and how this PR fixes it?

I think the PR description (on this PR) elucidates that sufficiently.

As for the likelihood of regressions here involving the runner scripts... well, it's just plain hard to be sure. The good news is that the runner scripts aren't the main way that people consume Scala.

We do have some backup in the form of scala-dist-smoketest (https://github.com/scala/scala-dist-smoketest). If we end up needing to correct some regression, that would be a logical place for a regression test.

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.

4 participants