Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
OLibutzki commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1987934181 > The release is still under vote. @gnodet Thanks for pushing that release ahead! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
reckart commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1985714575 @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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
reckart commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1985709598 https://lists.apache.org/list?d...@maven.apache.org:2024-3:vote -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
OLibutzki commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1985701031 > The release is still under vote. Thanks for the feedback... this is not a publicly visible process, right? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
gnodet commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1985677119 > > 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](https://github.com/apache/maven-filtering/releases/tag/maven-filtering-3.3.2), but the release did not find its way to Maven Central. Do you know why @gnodet? The release is still under vote. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
OLibutzki commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1985381882 > I'm doing a review of maven shared projects now and will do a release for this project also. Any progress, @slachiewicz? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
slachiewicz commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1888918418 I'm doing a review of maven shared projects now and will do a release for this project also. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-138328 > > 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 ... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
michael-o commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-129990 > 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
OLibutzki commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1884404140 Do you have any approx. arrival date for a release including this fix? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
elharo merged PR #77: URL: https://github.com/apache/maven-filtering/pull/77 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
akuhtz commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1863016485 Can this be merged? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on code in PR #77:
URL: https://github.com/apache/maven-filtering/pull/77#discussion_r1420419510
##
src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java:
##
@@ -189,20 +190,44 @@ public void filterResources(MavenResourcesExecution
mavenResourcesExecution) thr
// as destination
// see MNG-1345
File outputDirectory =
mavenResourcesExecution.getOutputDirectory();
-boolean outputExists = outputDirectory.exists();
-if (!outputExists && !outputDirectory.mkdirs()) {
+if (!outputDirectory.mkdirs() && !outputDirectory.exists()) {
throw new MavenFilteringException("Cannot create resource
output directory: " + outputDirectory);
}
if (resource.isFiltering()) {
isFilteringUsed = true;
}
-boolean ignoreDelta = !outputExists
-||
buildContext.hasDelta(mavenResourcesExecution.getFileFilters())
-||
buildContext.hasDelta(getRelativeOutputDirectory(mavenResourcesExecution));
-LOGGER.debug("ignoreDelta " + ignoreDelta);
-Scanner scanner = buildContext.newScanner(resourceDirectory,
ignoreDelta);
+boolean filtersFileChanges =
buildContext.hasDelta(mavenResourcesExecution.getFileFilters());
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
elharo commented on code in PR #77:
URL: https://github.com/apache/maven-filtering/pull/77#discussion_r1420408887
##
src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java:
##
@@ -189,20 +190,44 @@ public void filterResources(MavenResourcesExecution
mavenResourcesExecution) thr
// as destination
// see MNG-1345
File outputDirectory =
mavenResourcesExecution.getOutputDirectory();
-boolean outputExists = outputDirectory.exists();
-if (!outputExists && !outputDirectory.mkdirs()) {
+if (!outputDirectory.mkdirs() && !outputDirectory.exists()) {
throw new MavenFilteringException("Cannot create resource
output directory: " + outputDirectory);
}
if (resource.isFiltering()) {
isFilteringUsed = true;
}
-boolean ignoreDelta = !outputExists
-||
buildContext.hasDelta(mavenResourcesExecution.getFileFilters())
-||
buildContext.hasDelta(getRelativeOutputDirectory(mavenResourcesExecution));
-LOGGER.debug("ignoreDelta " + ignoreDelta);
-Scanner scanner = buildContext.newScanner(resourceDirectory,
ignoreDelta);
+boolean filtersFileChanges =
buildContext.hasDelta(mavenResourcesExecution.getFileFilters());
Review Comment:
filtersFileChanged? since "changes" sounds like it's the actual changes
instead of a boolean indicating whether things changed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on code in PR #77: URL: https://github.com/apache/maven-filtering/pull/77#discussion_r1420403517 ## src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java: ## @@ -321,19 +347,20 @@ public void filterResources(MavenResourcesExecution mavenResourcesExecution) thr } /** - * Get the encoding to use when filtering the specified file. Properties files can be configured to use a different - * encoding than regular files. + * Get the encoding to use when filtering the specified file. Properties files + * can be configured to use a different encoding than regular files. * - * @param file The file to check - * @param encoding The encoding to use for regular files + * @param file The file to check Review Comment: I now reverted all unrelated javadoc changes -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
elharo commented on code in PR #77: URL: https://github.com/apache/maven-filtering/pull/77#discussion_r1420397992 ## src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java: ## @@ -321,19 +347,20 @@ public void filterResources(MavenResourcesExecution mavenResourcesExecution) thr } /** - * Get the encoding to use when filtering the specified file. Properties files can be configured to use a different - * encoding than regular files. + * Get the encoding to use when filtering the specified file. Properties files + * can be configured to use a different encoding than regular files. * - * @param file The file to check - * @param encoding The encoding to use for regular files + * @param file The file to check Review Comment: Did spotless do that? That's new, i think, and a strike against spotless if so. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on code in PR #77: URL: https://github.com/apache/maven-filtering/pull/77#discussion_r1420394454 ## src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java: ## @@ -321,19 +347,20 @@ public void filterResources(MavenResourcesExecution mavenResourcesExecution) thr } /** - * Get the encoding to use when filtering the specified file. Properties files can be configured to use a different - * encoding than regular files. + * Get the encoding to use when filtering the specified file. Properties files + * can be configured to use a different encoding than regular files. * - * @param file The file to check - * @param encoding The encoding to use for regular files + * @param file The file to check Review Comment: I use `spoltless:apply` to format the file according to project rules, so not fully sure how to proceed, do you want me to revert the format changes from spotles? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
elharo commented on code in PR #77: URL: https://github.com/apache/maven-filtering/pull/77#discussion_r1420388359 ## src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java: ## @@ -321,19 +347,20 @@ public void filterResources(MavenResourcesExecution mavenResourcesExecution) thr } /** - * Get the encoding to use when filtering the specified file. Properties files can be configured to use a different - * encoding than regular files. + * Get the encoding to use when filtering the specified file. Properties files + * can be configured to use a different encoding than regular files. * - * @param file The file to check - * @param encoding The encoding to use for regular files + * @param file The file to check Review Comment: Probably better not to try to line these up. It's too hard to maintain. But we should make the initial letter lower case in keeping with Oracle guideline -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1846852881 All checks have passed! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1846830645 @lalmeras sounds reasonable, I have now adjust the code accordingly (and slightly restructured the flow), now my local build shows all test passing! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
lalmeras commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1844896414 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1844620007 @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... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
lalmeras commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1843346851 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 https://github.com/lalmeras/maven-filtering/commit/2525912aca62de2affd6e0d80616722454bd24a6 (I posted it on #83 :-( ) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1842459505 It currently fails on test-time will need to investigate what it wants here... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
lalmeras commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1837140171 @laeubi PR #83 with failing test cases and updated commit messages. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1837123822 @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! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
lalmeras commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1837123168 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1837114983 @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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
lalmeras commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1837113152 @laeubi Can you check https://github.com/lalmeras/maven-filtering/commit/dbe13673489c61a5c277b507310083bdcaa87c92#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 ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1837037738 @lalmeras great to see you make progress on this :+1: 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 change scope 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`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
lalmeras commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1836797752 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 ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
lalmeras commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1833477761 Done ! #82 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1833089113 > 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! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
lalmeras commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1832907067 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
lalmeras commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1831557819 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1831210021 @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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
lalmeras commented on PR #77:
URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1830852620
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1820426240 > @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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
mickaelistria commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1820418224 @laeubi would it be possible to add a unit test that showcases the issue and the proposed fix? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
pcdavid commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1787352103 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](https://github.com/eclipse-m2e/m2e-core/issues/1302) I was curious, and the last PR build is too old and the failure logs are not available anymore. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [MSHARED-1285] use an up-to-date scanner instead the newscanner [maven-filtering]
laeubi commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1774104149 > Is there any way to do thsi that doesn't use plexus at all? E.g. something in commons-io instead? obviously not... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MSHARED-1285 use an up-to-date scanner instead the newscanner [maven-filtering]
OLibutzki commented on PR #77: URL: https://github.com/apache/maven-filtering/pull/77#issuecomment-1752774319 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
