You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "reda-alaoui (via GitHub)" <gi...@apache.org> on 2024/08/21 14:39:01 UTC
[PR] [MBUILDCACHE-101] mvn test fails on missing classes after mvn compile leads to empty jars [maven-build-cache-extension]
reda-alaoui opened a new pull request, #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176
This PR makes sure any cached build that doesn't involve phase `package` is considered as invalid.
------
Following this checklist to help us incorporate your
contribution quickly and easily:
- [X] Make sure there is a [MBUILDCACHE JIRA issue](https://issues.apache.org/jira/browse/MBUILDCACHE) 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.
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] Format the pull request title like `[MBUILDCACHE-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MBUILDCACHE-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.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [X] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) 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](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "AlexanderAshitkin (via GitHub)" <gi...@apache.org>.
AlexanderAshitkin commented on code in PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#discussion_r1732039886
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -255,6 +255,11 @@ private CacheResult analyzeResult(CacheContext context, List<MojoExecution> mojo
build.getCacheImplementationVersion());
}
+ if (lifecyclePhasesHelper.isLaterPhaseThanBuild("package", build)) {
+ LOGGER.warn("Cached build doesn't include phase 'package', cannot restore");
Review Comment:
In the current implementation, the 'analyzeResult' returns partial success in lines 277-278 as expected.
A more accurate implementation would be to always attach the generated sources in `save` and process partial success more accurately. A build without a package phase should restore generated sources only and rerun all the missing lifecycle phases starting with compile to build missing classes.
The second concern is that we still process builds that will never be used in the 'save'. Save and restore should be consistent, and the situation when the cached builds are not usable should also be considered for improvement.
Looking at this fix, it seems that saving pre-packaged builds has no value unless cache properly supports compile phase caching. As a quick fix, this change seems legitimate.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "AlexanderAshitkin (via GitHub)" <gi...@apache.org>.
AlexanderAshitkin commented on code in PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#discussion_r1732039886
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -255,6 +255,11 @@ private CacheResult analyzeResult(CacheContext context, List<MojoExecution> mojo
build.getCacheImplementationVersion());
}
+ if (lifecyclePhasesHelper.isLaterPhaseThanBuild("package", build)) {
+ LOGGER.warn("Cached build doesn't include phase 'package', cannot restore");
Review Comment:
In the current implementation, the 'analyzeResult' returns partial success in lines 277-278 as expected.
More accurate implementation could support pre-compile phases - 'save' and restore generated sources. This will allow restoring missing sources from the cache and potentially saving time on the code generation.
The second concern is that we still process builds that will never be used in the 'save'. Save and restore should be consistent, and the situation when cached builds are not usable should also be considered for improvement.
As a quick fix, this change seems legitimate.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "jira-importer (via GitHub)" <gi...@apache.org>.
jira-importer commented on PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#issuecomment-2955467718
Resolve #314
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "reda-alaoui (via GitHub)" <gi...@apache.org>.
reda-alaoui commented on code in PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#discussion_r1734733542
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -255,6 +255,11 @@ private CacheResult analyzeResult(CacheContext context, List<MojoExecution> mojo
build.getCacheImplementationVersion());
}
+ if (lifecyclePhasesHelper.isLaterPhaseThanBuild("package", build)) {
+ LOGGER.warn("Cached build doesn't include phase 'package', cannot restore");
Review Comment:
@AlexanderAshitkin I agree with you.
At first I wanted to fix the extension to make it able to save cache state on phase < package. But there is this sentence in the [documentation](https://maven.apache.org/extensions/maven-build-cache-extension/usage.html):
> Once the extension is activated, the cache automatically kicks in on every package or higher phase.
I could not find the reason of this constraint, so I made sure this fix respects the latter. Also the fix should be quick to merge to prevent people from getting into trouble.
I also think this extension should not limit itself to phases >= `package`. We can still come back to that later.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "jira-importer (via GitHub)" <gi...@apache.org>.
jira-importer commented on PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#issuecomment-2955468279
Resolve #340
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "AlexanderAshitkin (via GitHub)" <gi...@apache.org>.
AlexanderAshitkin commented on code in PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#discussion_r1734690476
##########
src/test/java/org/apache/maven/buildcache/its/Issue105Test.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.buildcache.its;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+
+import org.apache.commons.compress.archivers.jar.JarArchiveEntry;
+import org.apache.commons.compress.archivers.jar.JarArchiveInputStream;
+import org.apache.maven.buildcache.its.junit.IntegrationTest;
+import org.apache.maven.it.VerificationException;
+import org.apache.maven.it.Verifier;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * @author Réda Housni Alaoui
+ */
+@IntegrationTest("src/test/projects/mbuildcache-105")
+class Issue105Test {
+
+ public static final String GENERATED_JAR = "target/simple-0.0.1-SNAPSHOT.jar";
+
+ @Test
+ void simple(Verifier verifier) throws VerificationException, IOException {
+ verifier.setAutoclean(false);
+
+ verifier.setLogFileName("../log-1.txt");
+ verifier.executeGoals(Arrays.asList("clean", "process-test-classes"));
+ verifier.verifyErrorFreeLog();
+
+ verifier.setLogFileName("../log-2.txt");
+ verifier.executeGoals(Arrays.asList("clean", "verify"));
+ verifier.verifyErrorFreeLog();
+ verifier.verifyTextInLog("Cached build doesn't include phase 'package', cannot restore");
+ verifier.verifyFilePresent(GENERATED_JAR);
Review Comment:
BUILT_JAR?
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "AlexanderAshitkin (via GitHub)" <gi...@apache.org>.
AlexanderAshitkin commented on code in PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#discussion_r1732039886
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -255,6 +255,11 @@ private CacheResult analyzeResult(CacheContext context, List<MojoExecution> mojo
build.getCacheImplementationVersion());
}
+ if (lifecyclePhasesHelper.isLaterPhaseThanBuild("package", build)) {
+ LOGGER.warn("Cached build doesn't include phase 'package', cannot restore");
Review Comment:
In the current implementation, the 'analyzeResult' returns partial success in lines 277-278 as expected.
More accurate implementation could support pre-compile phases - 'save' and restore generated sources and continue with compile. This will allow restoring missing sources from the cache and potentially saving time on the code generation. Could be handy in IDEs which could faster compile the project after a branch switch (if generated sources are restored).
The second concern is that we still process builds that will never be used in the 'save'. Save and restore should be consistent, and the situation when cached builds are not usable should also be considered for improvement.
Though overall, as a quick fix, this change seems legitimate, it feels like restoring generated sources and commencing with compilation would be more optimal.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "AlexanderAshitkin (via GitHub)" <gi...@apache.org>.
AlexanderAshitkin commented on code in PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#discussion_r1732039886
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -255,6 +255,11 @@ private CacheResult analyzeResult(CacheContext context, List<MojoExecution> mojo
build.getCacheImplementationVersion());
}
+ if (lifecyclePhasesHelper.isLaterPhaseThanBuild("package", build)) {
+ LOGGER.warn("Cached build doesn't include phase 'package', cannot restore");
Review Comment:
In the current implementation, the 'analyzeResult' returns partial success in lines 277-278 as expected.
More accurate implementation could support pre-compile phases - 'save' and restore generated sources and continue with compile. This will allow restoring missing sources from the cache and potentially saving time on the code generation. Could be handy in IDEs which could compile the project if generated sources are restored.
The second concern is that we still process builds that will never be used in the 'save'. Save and restore should be consistent, and the situation when cached builds are not usable should also be considered for improvement.
Though overall, as a quick fix, this change seems legitimate, it feels like restoring generated sources and commencing with compilation would be more optimal.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "jira-importer (via GitHub)" <gi...@apache.org>.
jira-importer commented on PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#issuecomment-2955451830
Resolve #314
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "AlexanderAshitkin (via GitHub)" <gi...@apache.org>.
AlexanderAshitkin commented on code in PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#discussion_r1732039886
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -255,6 +255,11 @@ private CacheResult analyzeResult(CacheContext context, List<MojoExecution> mojo
build.getCacheImplementationVersion());
}
+ if (lifecyclePhasesHelper.isLaterPhaseThanBuild("package", build)) {
+ LOGGER.warn("Cached build doesn't include phase 'package', cannot restore");
Review Comment:
In the current implementation, the 'analyzeResult' returns partial success in lines 277-278 as expected.
More accurate implementation could support pre-compile phases - 'save' and restore generated sources. This will allow restoring missing sources from the cache and potentially saving time on the code generation. Could be handy in IDEs which could compile the project if generated sources are restored.
The second concern is that we still process builds that will never be used in the 'save'. Save and restore should be consistent, and the situation when cached builds are not usable should also be considered for improvement.
Though overall, as a quick fix, this change seems legitimate.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "jira-importer (via GitHub)" <gi...@apache.org>.
jira-importer commented on PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#issuecomment-2955453187
Resolve #340
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "reda-alaoui (via GitHub)" <gi...@apache.org>.
reda-alaoui closed pull request #176: [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars
URL: https://github.com/apache/maven-build-cache-extension/pull/176
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "reda-alaoui (via GitHub)" <gi...@apache.org>.
reda-alaoui commented on PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#issuecomment-2348565410
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "AlexanderAshitkin (via GitHub)" <gi...@apache.org>.
AlexanderAshitkin commented on code in PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#discussion_r1732039886
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -255,6 +255,11 @@ private CacheResult analyzeResult(CacheContext context, List<MojoExecution> mojo
build.getCacheImplementationVersion());
}
+ if (lifecyclePhasesHelper.isLaterPhaseThanBuild("package", build)) {
+ LOGGER.warn("Cached build doesn't include phase 'package', cannot restore");
Review Comment:
The expected behavior is that when two consecutive 'mvn compile' commands are used, the second 'mvn compile' should use the cache. I'm concerned that this implementation might lead to a performance regression. A more accurate implementation would be to always attach the generated sources in `save` and process partial success more accurately.
In the current implementation, the 'analyzeResult' returns partial success here (see lines 277-278) as expected. The fix should be made in the partial success processing logic. A build without a package phase should restore generated sources only and rerun all the missing lifecycle phases to build artifacts.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "AlexanderAshitkin (via GitHub)" <gi...@apache.org>.
AlexanderAshitkin commented on code in PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#discussion_r1732039886
##########
src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java:
##########
@@ -255,6 +255,11 @@ private CacheResult analyzeResult(CacheContext context, List<MojoExecution> mojo
build.getCacheImplementationVersion());
}
+ if (lifecyclePhasesHelper.isLaterPhaseThanBuild("package", build)) {
+ LOGGER.warn("Cached build doesn't include phase 'package', cannot restore");
Review Comment:
In the current implementation, the 'analyzeResult' returns partial success in lines 277-278 as expected.
A more accurate implementation would be to always attach the generated sources in `save` and process partial success more accurately. A build without a package phase should restore generated sources only and rerun all the missing lifecycle phases starting with compile to build missing compile sources.
The second concern is that we still process builds that will never be used in the 'save'. Save and restore should be consistent, and the situation when the cached builds are not usable should also be considered for improvement.
Looking at this fix, it seems that saving pre-packaged builds has no value unless cache properly supports compile phase caching. As a quick fix, this change seems legitimate.
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MBUILDCACHE-105] Executing 'clean process-test-classes' then 'clean verify' leads to empty jars [maven-build-cache-extension]
Posted by "reda-alaoui (via GitHub)" <gi...@apache.org>.
reda-alaoui commented on PR #176:
URL: https://github.com/apache/maven-build-cache-extension/pull/176#issuecomment-5778545089
Fixed upstream
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org