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

Add test case for invalid compilation against fragments from P2 repo #4293

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HeikoKlare
Copy link
Contributor

@HeikoKlare HeikoKlare commented Sep 23, 2024

When compiling a fragment for whose hose a fragment exists in a P2 repo referenced by the build (even if the platform filter does not match), the classes from that fragment are put on the classpath, such that code in the currently compiled fragment is illegally compiled against that fragment code.

This change adds a test case that demonstrates the behavior via a fragment that uses code from another fragment, which is must not depend on.

See the discussion in eclipse-platform/eclipse.platform.releng.aggregator#2360 (comment)
Interestingly, this test also fails with 4.0.8 while the SWT build issues this is supposed to be a regression test for did not appear before 4.0.9. Still, the situation in this test is, in my understanding, unexpected, and should cover the case we have in SWT as well.

Copy link

github-actions bot commented Sep 23, 2024

Test Results

  603 files    603 suites   4h 37m 19s ⏱️
  431 tests   422 ✅  7 💤 2 ❌
1 293 runs  1 267 ✅ 22 💤 4 ❌

For more details on these failures, see this check.

Results for commit c742b40.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Member

laeubi commented Sep 24, 2024

Thanks Heiko, I'll try to look into this.

@HeikoKlare
Copy link
Contributor Author

Thank you, Christoph! That would be really great. I am afraid my knowledge in Tycho is not deep enough to provide an according enhancement soon, at least not a proper one.

@@ -12,6 +12,9 @@
*******************************************************************************/
package org.eclipse.tycho.test.eeProfile;

import static org.junit.jupiter.api.Assertions.assertThrows;

import org.apache.bcel.verifier.exc.VerificationException;
Copy link
Member

Choose a reason for hiding this comment

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

bcel. VerificationException ? Are you sure? I would assume this has to be org.apache.maven.it.VerificationException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's wrong. It has to be org.apache.maven.it.VerificationException. I will have a look.

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've corrected the import and the test still fails as expected.

As an additional information for investigation: when executed locally with debug output, you see the unexpected classpath entry for the unmatching fragment when compiling the gtk.linux.x86 one:

[DEBUG] Classpath: [/Users/heikoklare/dev/eclipse/tycho/git/tycho/tycho-its/projects/eeProfile.resolution.fragments.unmatchinginp2/compiler.fragments.unmatchinginp2/.
 /Users/heikoklare/.m2/repository/p2/osgi/bundle/compiler.fragments.unmatchinginp2.unmatching/1.0.0.202409231244/compiler.fragments.unmatchinginp2.unmatching-1.0.0.202409231244.jar
 /Users/heikoklare/dev/eclipse/tycho/git/tycho/tycho-its/projects/eeProfile.resolution.fragments.unmatchinginp2/compiler.fragments.unmatchinginp2.gtk.linux.x86/target/classes
 /Users/heikoklare/.m2/repository/org/osgi/org.osgi.service.cm/1.6.1/org.osgi.service.cm-1.6.1.jar[+org/osgi/service/cm/annotations/*:?**/*]
 /Users/heikoklare/.m2/repository/org/osgi/org.osgi.annotation.bundle/1.1.1/org.osgi.annotation.bundle-1.1.1.jar[+org/osgi/annotation/bundle/*:?**/*]
 /Users/heikoklare/.m2/repository/org/osgi/org.osgi.annotation.versioning/1.1.2/org.osgi.annotation.versioning-1.1.2.jar[+org/osgi/annotation/versioning/*:?**/*]
 /Users/heikoklare/.m2/repository/org/osgi/org.osgi.service.metatype.annotations/1.4.1/org.osgi.service.metatype.annotations-1.4.1.jar[+org/osgi/service/metatype/annotations/*:?**/*]
 /Users/heikoklare/.m2/repository/org/osgi/org.osgi.service.event/1.4.1/org.osgi.service.event-1.4.1.jar[+org/osgi/service/event/annotations/*:?**/*]
 /Users/heikoklare/.m2/repository/org/osgi/org.osgi.service.component.annotations/1.5.1/org.osgi.service.component.annotations-1.5.1.jar[+org/osgi/service/component/annotations/*:?**/*]]

When compiling a fragment for whose hose a fragment exists in a P2 repo
referenced by the build (even if the platform filter does not match),
the classes from that fragment are put on the classpath, such that code
in the currently compiled fragment is illegally compiled against that
fragment code.

This change adds a test case that demonstrates the behavior via a
fragment that uses code from another fragment, which is must not depend
on.
@laeubi
Copy link
Member

laeubi commented Oct 9, 2024

@HeikoKlare I see the unmatching on the classpath in the debug output but the compile does not fails, is this expected?

Classpath: [eeProfile.resolution.fragments.unmatchinginp2/compiler.fragments.unmatchinginp2/.
repository/p2/osgi/bundle/compiler.fragments.unmatchinginp2.unmatching/1.0.0.202409231244/compiler.fragments.unmatchinginp2.unmatching-1.0.0.202409231244.jar
eeProfile.resolution.fragments.unmatchinginp2/compiler.fragments.unmatchinginp2.gtk.linux.x86/target/classes
repository/org/osgi/org.osgi.annotation.versioning/1.1.2/org.osgi.annotation.versioning-1.1.2.jar[+org/osgi/annotation/versioning/*:?**/*]
repository/org/osgi/org.osgi.service.cm/1.6.1/org.osgi.service.cm-1.6.1.jar[+org/osgi/service/cm/annotations/*:?**/*]
repository/org/osgi/org.osgi.annotation.bundle/1.1.1/org.osgi.annotation.bundle-1.1.1.jar[+org/osgi/annotation/bundle/*:?**/*]
repository/org/osgi/org.osgi.service.event/1.4.1/org.osgi.service.event-1.4.1.jar[+org/osgi/service/event/annotations/*:?**/*]
repository/org/osgi/org.osgi.service.metatype.annotations/1.4.1/org.osgi.service.metatype.annotations-1.4.1.jar[+org/osgi/service/metatype/annotations/*:?**/*]
repository/org/osgi/org.osgi.service.component.annotations/1.5.1/org.osgi.service.component.annotations-1.5.1.jar[+org/osgi/service/component/annotations/*:?**/*]]

the integration test seem to indicate that a compile error is about to be expected, also if I compare 4.0.8 debug output with 4.0.9 debug output I get the same result regarding the classpath output.

@HeikoKlare
Copy link
Contributor Author

I see the unmatching on the classpath in the debug output but the compile does not fails, is this expected?

Yes (depending what "expected" refers to): the code for the test is written in a way that compilation should fail when fragments are considered correctly. I.e., there is a class in the fragment-to-be-built that depends on a class in the unmatching fragment. Currently, the compilation succeeds, but actually a failure is expected. The test case is written in a way that it expects a compilation failure, which is why the test fails due to the compilation succeeding.

the integration test seem to indicate that a compile error is about to be expected, also if I compare 4.0.8 debug output with 4.0.9 debug output I get the same result regarding the classpath output.

This issue seems to be existent longer than 4.0.9. The change in 4.0.9 (#3928) just changed the way in which dependencies from P2 repos are resolved/considered, such that this existent (undetected) issue now leads to a visible issue / derivation of behavior. At least this is my current understanding of the problem. I was not able to provide a test for the exact issue happening with 4.0.9 (such that the test succeeds with 4.0.8). It would be more complicated to define and I expect the issue shown with this test to be the root cause for the issue appearing with 4.0.9, as the actual difference between 4.0.8 and 4.0.9 builds is that with 4.0.9 wrong fragments appear on the classpath (like in this test) while they were not present on the classpath with 4.0.8.

@laeubi
Copy link
Member

laeubi commented Oct 15, 2024

Thanks for the clarification.

@laeubi
Copy link
Member

laeubi commented Nov 4, 2024

@HeikoKlare I now added this testcase together with a fix to this PR:

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.

3 participants