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

Limit ValidateClassFilesTest to scan only JARs referenced from modules #4775

Merged

Conversation

matthiasblaesing
Copy link
Contributor

With the addition of jakarta ee 10 API jars it was found, that there is an implicit assumption, that all JARs not referenced by modules have to be JDK 8 compatible. In the case of jakarta ee 10 this is an invalid assumption as it explicitly does not support JDK 8. The module that introduces the binaries correctly declares a JDK 11 dependeny, but at runtime the module <-> binary association is lost.

To prevent false positives, the test must be limited to JARs, that are directly referenced from modules.

With the addition of jakarta ee 10 API jars it was found, that there is
an implicit assumption, that all JARs not referenced by modules have
to be JDK 8 compatible. In the case of jakarta ee 10 this is an
invalid assumption as it explicitly does not support JDK 8. The module
that introduces the binaries correctly declares a JDK 11 dependeny, but
at runtime the module <-> binary association is lost.

To prevent false positives, the test must be limited to JARs, that are
directly referenced from modules.
@matthiasblaesing matthiasblaesing added this to the NB16 milestone Oct 11, 2022
@matthiasblaesing matthiasblaesing added the ci:all-tests [ci] enable all tests label Oct 11, 2022
@apache apache locked and limited conversation to collaborators Oct 11, 2022
@apache apache unlocked this conversation Oct 11, 2022
@matthiasblaesing
Copy link
Contributor Author

@neilcsmith-net @jtulach you two worked previously on this test. This change is triggered by the commit validation errors reported in #4692 (Commit da98cbd). From my observation the problem is, that the test assumes, that all JARs, that may need a java version higher than 8 are referenced by modules (either as module itself or as dependency). This is not true for bundled jar files. In the referenced PR the bundled library is Jakarta EE 10, which has a minimum baseline of JDK 11.

Copy link
Member

@neilcsmith-net neilcsmith-net left a comment

Choose a reason for hiding this comment

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

I think this is OK as a quick fix. I acknowledge that my addition was also a quick fix! Ideally we should probably add bundled JARs along with the result of module.getAllJars() but I wasn't sure of the best way to access this information in the test (and didn't need it to get it to pass then!)

@mbien
Copy link
Member

mbien commented Oct 12, 2022

restarting tests since master is green again.

@apache apache locked and limited conversation to collaborators Oct 12, 2022
@apache apache unlocked this conversation Oct 12, 2022
@jGauravGupta jGauravGupta merged commit 3cbe596 into apache:master Oct 14, 2022
@mbien mbien added the Platform [ci] enable platform tests (platform/*) label Dec 3, 2022
Chris2011 pushed a commit to Chris2011/netbeans that referenced this pull request Dec 15, 2022
apache#4775)

With the addition of jakarta ee 10 API jars it was found, that there is
an implicit assumption, that all JARs not referenced by modules have
to be JDK 8 compatible. In the case of jakarta ee 10 this is an
invalid assumption as it explicitly does not support JDK 8. The module
that introduces the binaries correctly declares a JDK 11 dependeny, but
at runtime the module <-> binary association is lost.

To prevent false positives, the test must be limited to JARs, that are
directly referenced from modules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-tests [ci] enable all tests Platform [ci] enable platform tests (platform/*)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants