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 "m2e plugin sometimes 'loses' resources" (#1511) #1629

Merged
merged 4 commits into from
Dec 22, 2023

Conversation

kohlschuetter
Copy link
Contributor

The Java Builder may delete files from the project output directory that need to be re-created by the m2e Maven Builder.

With commit 8e5cd49 (a fix for #1275), any changes to the project output directory were ignored, leading to unexpected errors when running an application after modifying the project POM: resources were missing from the target classpath, leading all sorts of unexpected program behavior.

This change allows marking these changes as relevant, as long as the following conditions are met:

  • The expected resource no longer exists (i.e., it was deleted by another builder, plugin or outside process)
  • The modification applies to a resource in the classes or test-classes folder (i.e., outputLocation/testOutputLocation)

#1511 #1275

The Java Builder may delete files from the project output directory that
need to be re-created by the m2e Maven Builder.

With commit 8e5cd49 (a fix for eclipse-m2e#1275),
any changes to the project output directory were ignored, leading to
unexpected errors when running an application after modifying the
project POM: resources were missing from the target classpath, leading
all sorts of unexpected program behavior.

This change allows marking these changes as relevant, as long as the
following conditions are met:

- The expected resource no longer exists (i.e., it was deleted by
  another builder, plugin or outside process)
- The modification applies to a resource in the classes or test-classes
  folder (i.e., outputLocation/testOutputLocation)

eclipse-m2e#1511
eclipse-m2e#1275
@kohlschuetter
Copy link
Contributor Author

CI failure: "Baseline problems found! Project version ..." can be fixed with #1630

@laeubi
Copy link
Member

laeubi commented Dec 20, 2023

CI failure: "Baseline problems found! Project version ..." can be fixed with #1630

Please include any version changes in your PR here.

@@ -219,7 +225,11 @@ private boolean hasRelevantDelta(IMavenProjectFacade projectFacade, IResourceDel
IPath fullPath = delta.getFullPath();
if(buildOutputLocation.isPrefixOf(fullPath)) {
//anything in the build output is not interesting for a change as it is produced by the build
//lets see if there are more interesting parts...
// ... unless a classpath resource that existed before has been deleted, possibly by another builder
if(!resource.exists() && isOutputOrTestOutput.test(fullPath)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(!resource.exists() && isOutputOrTestOutput.test(fullPath)) {
if(isOutputOrTestOutput.test(fullPath) && !resource.exists()) {

checking the prefix is probably a bit cheaper than checking all resources for existence.

Copy link
Member

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

The change looks good in general now and it seems a valid assumption that if the output folder is gone something must be happen.

But thinking more about it I think this is more fundamentally problem even though it is fixing things for resource plugin others might behave odd, one example is the maven-bundle-plugin or bnd-maven-plugins that might also place resources there.

Because of this, please apply the following changes:

  1. hasRelevantDelta should return an enum that has the values: DELTA (the current 'true' return value), FULL (when detecting the new output folder is gone), IRRELEVANT (the current 'false' return value.
  2. In setupProjectBuildContext a value of FULL should result in a null build delta passed so we make sure a full build is executed and not an incremental one.

@kohlschuetter
Copy link
Contributor Author

@laeubi I think your recently requested changes are beyond the scope of the bug, and therefore better suited for a separate refactoring. I also feel that it's more reasonable if you implement these changes the way you envision them, it will save us both unnecessary back and forth.

@laeubi laeubi merged commit 2ae34a5 into eclipse-m2e:master Dec 22, 2023
2 checks passed
@HannesWell HannesWell added this to the 2.6.0 milestone Feb 17, 2024
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.

None yet

3 participants