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

Fix classpath issues in ClasspathJrt and model tests #3108

Merged

Conversation

stephan-herrmann
Copy link
Contributor

@stephan-herrmann stephan-herrmann merged commit 3e287dc into eclipse-jdt:master Oct 17, 2024
10 checks passed
@stephan-herrmann stephan-herrmann deleted the classpathIssues branch October 17, 2024 16:37
Comment on lines +295 to +301
ClasspathJrt classpathJrt = new ClasspathJrt(new File(convertPathSeparators(jdkHome)), true, accessRuleSet, null);
try {
classpathJrt.initialize();
} catch (IOException e) {
// Broken entry, but let clients have it anyway.
}
return classpathJrt;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged this already to fix eclipse-jdt/eclipse.jdt.ui#1722 for the next I-Build, but still I'd like to get a word from people having worked in this area, like @jukzi @jarthana

ClasspathJrt and friends don't seem to be usable before initialize() is called, but I could not see what kind of strategy was intended.

Is initialization supposed to happen on demand or eagerly? Neither strategy seems to be implemented consistently.

Backing up one step: is there reason to create a ClasspathJrt without initialization? Sure, instantiation is faster without, but is instantiation without initialization meaningful?

Copy link
Member

Choose a reason for hiding this comment

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

I definitely remember that initialize() should happen before everything else. But then I also remember us having few issues in this area and may have had some back-forth changes.

@stephan-herrmann
Copy link
Contributor Author

Fix test's getJCL15PlusLibraryIfNeeded()

For posterity a word on why I made this change: when running model tests with -Dcompliance=23 or any setting that doesn't include 1.8, then ClasspathTest.testBug576735a would throw NPE because in this configuration getJCL15PlusLibraryIfNeeded() returned null, simply because the requested "1.8" is not in the set of compliances specified via the system property. I'm quite sure that when explicitly requesting the JCL lib for a specific compliance, then that library should be provided (and not hardcoded "1.8"), no matter what system property was passed in.

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.

VisitorTest failing since I20241015-1820
2 participants