[MSHARED-1285] use an up-to-date scanner instead the newscanner

[laeubi]

Currently it could happen that the scanner misses changed files (because they are not part of the delta) or copies files even if they have not changed (e.g. because the output has changes).

This uses now a different approach, instead of only handling the delta files, we scan all inputs and compare if they are up-to-date with the output.

Following this checklist to help us incorporate your
contribution quickly and easily:

• Make sure there is a JIRA issue filed
  for the change (usually before you start working on it). Trivial changes like typos do not
  require a JIRA issue. Your pull request should address just this issue, without
  pulling in other changes.
• Each commit in the pull request should have a meaningful subject line and body.
• Format the pull request title like [MSHARED-XXX] - Fixes bug in ApproximateQuantiles,
  where you replace MSHARED-XXX with the appropriate JIRA issue. Best practice
  is to use the JIRA issue title in the pull request title and in the first line of the
  commit message.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Run mvn clean verify -Prun-its to make sure basic checks pass. A more thorough check will
  be performed on your pull request automatically.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• In any other case, please file an Apache Individual Contributor License Agreement.

[OLibutzki]

Any chances to get some progress into this PR? From an Eclipse user perspective this issue is really annoying as some resources are not copied to the target/classes folder.

//cc @khmarbaise

[elharo]

tests are failing.

Is there any way to do this that doesn't use plexus at all? E.g. something in commons-io instead?

[laeubi]

Is there any way to do thsi that doesn't use plexus at all? E.g. something in commons-io instead?

obviously not...

[pcdavid]

FWIW, the (single) test failure I get with this PR is this one:

[ERROR] Tests run: 4, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.162 s <<< FAILURE! - in org.apache.maven.shared.filtering.IncrementalResourceFilteringTest
[ERROR] org.apache.maven.shared.filtering.IncrementalResourceFilteringTest.testSimpleIncrementalFiltering  Time elapsed: 0.031 s  <<< FAILURE!
junit.framework.ComparisonFailure: expected:<[]time> but was:<[no]time>
	at junit.framework.Assert.assertEquals(Assert.java:100)
	at junit.framework.Assert.assertEquals(Assert.java:107)
	at junit.framework.TestCase.assertEquals(TestCase.java:260)
	at org.apache.maven.shared.filtering.IncrementalResourceFilteringTest.assertTime(IncrementalResourceFilteringTest.java:158)
	at org.apache.maven.shared.filtering.IncrementalResourceFilteringTest.testSimpleIncrementalFiltering(IncrementalResourceFilteringTest.java:69)

I don't know anything about this project/codebase, but as an Eclipse user impacted by this I was curious, and the last PR build is too old and the failure logs are not available anymore.

[mickaelistria]

@laeubi would it be possible to add a unit test that showcases the issue and the proposed fix?

[laeubi]

@laeubi would it be possible to add a unit test that showcases the issue and the proposed fix?

It should be possible to reproduce the problem a testcase should be setup like this:

1. BuildContext reports an incremental build
2. There should be resource A + B
3. The resource target folder is empty
4. BuildContext only contains a change for A

Result is only A is copied (as it has a change) but B is missing because the Mojo assumes that a file in resource target can never vanish and do assume it will get a change event for that, what can not be guaranteed as a file can always be deleted out of the control of maven / IDE /...

Another case is that you modify the target file (but not the source) then the mojo will never update that file as long as one do not modify the source file.

[lalmeras]

I dig into the test issue.

Test verifies the following behavior:

• two files with resource filtering are filtered; both files are copied to target with ${time} replaced by time
• one file is touched and an incremental build with time=notime is launched
• expected behavior is that only the touched file is updated; other file keeps time value (instead of notime)
• after proposed fix, both files are processed

I think there is an issue with the test implementation. TestIncrementalBuildContext.isUptodate(File, File) (that is called by custom DirectoryScanner#isSelected, cf buildContext.isUptodate(...)) implementation always return false, because hasDelta(target) always return true, because hasDelta fails to resolve target path as a source path (cf getRelpath).

Not sure why, but the original code does not trigger TestIncrementalBuildContext.isUptodate(). Other tests from project does not call this method either.

So the issue for this test failure is in TestIncrementalBuildContext (org.sonatype.plexus:plexus-build-api:tests). Not sure how to handle this.

---

• isUptodate : https://github.com/codehaus-plexus/plexus-build-api/blob/caf6847f01b92c7e5e4c6f2afe455b72ec117f16/src/test/java/org/sonatype/plexus/build/incremental/test/TestIncrementalBuildContext.java#L193
• getRelpath : https://github.com/codehaus-plexus/plexus-build-api/blob/caf6847f01b92c7e5e4c6f2afe455b72ec117f16/src/test/java/org/sonatype/plexus/build/incremental/test/TestIncrementalBuildContext.java#L123 -> basedir is src/test/units-files/incremental

[laeubi]

@lalmeras many many thanks for looking into the test!

So the issue for this test failure is in TestIncrementalBuildContext (org.sonatype.plexus:plexus-build-api:tests). Not sure how to handle this.

This is actually deprecated/removed in later versions exactly because no one can know what the test wants to assert... so the best would be to simply copy it in the plugin here and adjust as needed, or even better using a mock.

[lalmeras]

Already done it (copy/fix TestIncrementalBuildContext). It doesn't work out of the box as there is other tests that rely on this bug to work. And another bug in getRelpath.

But I agree the best option is to retrieve/fix/adapt TestIncrementalBuildContext so that the 4 tests that use it work and actually test the effective filtering behavior. Just not as simple as it seems.

I plan to work on it. I'll keep this issue updated about my work on it.

Once the tests fixed, I think it'll be easier to write a test exposing the current issue.

[lalmeras]

Here is a branch with my proposal : https://github.com/lalmeras/maven-filtering/commits/MSHARED-1285

• first commit is a test refactor applied to existing master codebase
• second commit applies @laeubi proposal. I restore original ignoreDelta when filters or output directory trigger hasDelta=true, as this is an expected behaviour from tests

Both commits built with success with java 8 / java 17 / mvn 3.8.

I finally understand why old tests work. It is because they relied only on Scanner behavior, and not isUptodate / hasDelta methods. So they accommodate nicely with the buggy TestIncrementalBuildContext.

No work done on this issue's testcase. I can work on it if this first proposal seems OK.

I do not open another PR; feel free to integrate my proposal in this PR.

[laeubi]

I do not open another PR; feel free to integrate my proposal in this PR.

@lalmeras just open a PR for the testcases I think that can be easier to manage also we then have direct PR validation!

[lalmeras]

Done ! #82

[lalmeras]

I update my branch https://github.com/lalmeras/maven-filtering/commits/MSHARED-1285 with the following change :

• rebased on origin/master with the refactored tests
• I added a test-case demonstrating the usecase described by @laeubi : if target files are removed, an incremental build fails to regenerate missing resources

Another case is that you modify the target file (but not the source) then the mojo will never update that file as long as one do not modify the source file.

@laeubi I'm not sure to understand what is the current / expected behavior for this use-case. Can you give a more precise description ?

[laeubi]

@lalmeras great to see you make progress on this 👍

For the second case assume the following:

1. A resource (e.g. source.txt) was recently copied to the output folder output
2. There is another resource (e.g. source2.txt) also copied to the output folder output
3. Now it happens that output/source.txt is modified e.g. by the user outside the scope of changes
4. Now source2.txt is modified and only this edit is part of the changes reported

What happens now is that source2.txt is copied correctly to folder output as it is part of the changed files, but source.txt is not copied even though it is out-of-date with output/source.txt.

[lalmeras]

@laeubi Can you check lalmeras@dbe1367#diff-a52a7b8a0ff570c181fe18ab238b693c21243d40870dd2d8178340247755bba5R152 ?

Pay attention that my test assumes that BuildContext can reply appropriately #isUptodate = false for the updated target file. My stub handles this correctly, but I think that eclipse inplementation of BuildContext will not provide the expected result : https://github.com/eclipse-m2e/m2e-core/blob/348ef88b0d92833eff038d721a98ccf50e9ef4fd/org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/builder/plexusbuildapi/EclipseIncrementalBuildContext.java#L230

Am I missing something ?

[laeubi]

@lalmeras I think you are right that m2e assumes that if the target is newer than the source it is up-todate... but I think that can be ignored for now as this is an implementation detail of the context, the important part is that if isUptodate() return false, the file should be copied again.

[lalmeras]

Thank you for your feedback.

@laeubi Do you want to update your PR with my proposal so we now both have unit test demonstrating your use case (and failing with the current codebase) and your fix.

[laeubi]

@lalmeras can you open a PR here so we see the test is currently failing? I'll then fetch the PR and add my one on top of it to prove it fixes the failing test!

[lalmeras]

@laeubi PR #83 with failing test cases and updated commit messages.

[laeubi]

It currently fails on test-time will need to investigate what it wants here...

[lalmeras]

They are test-cases that handle filter modification. Expected behavior (checked by these tests) is to perform a full-build (this is the case where ignoreDelta := true is delta is found on filters).

I address this issue in my previous work by adapting your original fix (restoring ignoreDelta behavior, using ignoreDelta to adapt file scan) : see this commit lalmeras@2525912

(I previously posted this comment on #83 :-( )

[laeubi]

@lalmeras thanks for clarification, thinking more about it I wonder if ignoreDelta is actually useful for anything given we use isUptodate(), because for me it looks like ignoreDelta is some hack to try to solve the issues we see here in some cases and are now fixed by using an isUptodate scanner... What do you think?

[lalmeras]

I think ignoreDelta for this case is a well-founded behavior. It checks specifically filter files (not filtered files). If source for filters values is changed, I think it is expected that all resources are refreshed, hence the ignoreDelta.

isUptodate usage will not allow you to to reproduce this behavior during the scanning time.

[laeubi]

@lalmeras sounds reasonable, I have now adjust the code accordingly (and slightly restructured the flow), now my local build shows all test passing!

[laeubi]

All checks have passed!

[akuhtz]

Can this be merged?

[OLibutzki]

Do you have any approx. arrival date for a release including this fix?

[michael-o]

Do you have any approx. arrival date for a release including this fix?

We do not plan releases. Someone has to stand up as RM for this.

[laeubi]

Do you have any approx. arrival date for a release including this fix?

We do not plan releases. Someone has to stand up as RM for this.

Then the question is maybe is there someone who can drive the release and is willing to do so ...

[slachiewicz]

I'm doing a review of maven shared projects now and will do a release for this project also.

[OLibutzki]

I'm doing a review of maven shared projects now and will do a release for this project also.

Any progress, @slachiewicz?

There is a 3.3.2 tag, but the release did not find its way to Maven Central. Do you know why @gnodet?

[gnodet]

I'm doing a review of maven shared projects now and will do a release for this project also.

Any progress, @slachiewicz?

There is a 3.3.2 tag, but the release did not find its way to Maven Central. Do you know why @gnodet?

The release is still under vote.

[OLibutzki]

The release is still under vote.

Thanks for the feedback... this is not a publicly visible process, right?

[reckart]

https://lists.apache.org/list?dev@maven.apache.org:2024-3:vote

[reckart]

@OLibutzki the development process of Apache projects generally is public - you can help by testing and casting your vote as well - just reply to the voting mail. However, unless you are part of the project management committee, you vote will not count as binding. A release vote is successful with at least 3 +1 votes after at least 72h and no -1 vote. See https://www.apache.org/foundation/voting.html

[OLibutzki]

The release is still under vote.

@gnodet Thanks for pushing that release ahead!