Skip to content

Conversation

jlahoda
Copy link
Contributor

@jlahoda jlahoda commented May 20, 2018

-accepting duplicate ClassPath entries with module-infos
-filtering out options that don't match the validated source level

Fixing two problems with the given project - with JDK 10 platform, the sources ClassPath in ModuleClassPaths.ModuleInfoClassPathImplementation has a duplicated entry, leading to having the module-info.java twice in the list of files to which the listener should be added, leading to the reported exception. Proposed fix is to use LinkedHashSet to avoid the duplication. Having duplicated entries in the CP may not be nice, but I suspect it is not reasonable to try to force a constraint that the entries must not be duplicated.

With platform <JDK 10/9, there are multiple problems, but the biggest one is that there are additional compiler options, like --add-reads (auto-injected to make tests work?), but the source level is downgraded below 9, where these options are not allowed and crash with an exception. The proposed (partial) fix is to filter the 9-specific options out when the validated source level < 9.

Any ideas/opinions?

-accepting duplicate ClassPath entries with module-infos
-filtering out options that don't match the validated source level
@jlahoda jlahoda requested review from dbalek and geertjanw May 20, 2018 06:12
Copy link
Member

@geertjanw geertjanw left a comment

Choose a reason for hiding this comment

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

Thanks, great, so it sounds like things are a bit fragile between Java releases. If this fixes this particular problem, while not breaking anything, and we can anticipate there'll be other similar fixes needed over Java releases, let's try to be aware of that and merge this one.

@geertjanw
Copy link
Member

Can we merge this one?

@emilianbold
Copy link
Member

I don't know this whole area but the patch kinda makes sense to me.

@lkishalmi
Copy link
Contributor

@geertjanw @emilianbold Just pinging on this as the PR for one of our Blocker issues. Also when it hits master there shall be another PR or merge to release90 as well.

@geertjanw
Copy link
Member

OK, merging.

@JaroslavTulach JaroslavTulach self-requested a review June 5, 2018 11:38
@geertjanw geertjanw merged commit 9295f7b into apache:master Jun 5, 2018
JaroslavTulach pushed a commit to JaroslavTulach/netbeans that referenced this pull request Jun 5, 2018
JaroslavTulach pushed a commit to JaroslavTulach/netbeans that referenced this pull request Jun 5, 2018
jlahoda pushed a commit that referenced this pull request Jun 5, 2018
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