You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "Giovds (via GitHub)" <gi...@apache.org> on 2023/05/10 15:01:52 UTC

[GitHub] [maven] Giovds opened a new pull request, #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Giovds opened a new pull request, #1105:
URL: https://github.com/apache/maven/pull/1105

   This PR removes consumer*pom files from previous runs when the new consumer pom file is created. This way the build directory is not flooded with lots of `consumer*pom` files after a couple of runs.
   
   ---
   
   Following this checklist to help us incorporate your
   contribution quickly and easily:
   
    - [x] Make sure there is a [MNG-7740](https://issues.apache.org/jira/browse/MNG-7740) 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 `[MNG-XXX] SUMMARY`,
         where you replace `MNG-XXX` and `SUMMARY` with the appropriate JIRA issue.
    - [x] Also format the first line of the commit message like `[MNG-XXX] SUMMARY`.
          Best practice is to use the JIRA issue title in both 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.
    - [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)
   
    - [x] 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


[GitHub] [maven] gnodet commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1195672284


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);

Review Comment:
   > > Also, I think this removing _all_ matching files from the temporary directory is problematic. If we have two concurrent builds running, one of the files will be lost. This may be done only when `buildDirectory != null`.
   > 
   > It would be a problem. I don't think we can safely delete any files in that case without introducing too much complexity, so the behaviour would be to just let the files live in the temp dir and _maybe_ get cleaned up by the OS. Some do and some don't.
   
   There may be a better alternative.  I wonder if we could just add a `@PreDestroy` method that would keep a list of generated files (be it in the temp directory or in the build directory) and delete them when the bean is destroyed...
   
   > I didn't know the temp dir would/could be used as build directory (I just looked and the files were indeed there). Out of curiosity; when would this be the case?
   
   Actually, I did not know either, I just noticed it while reviewing 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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] mthmulders commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "mthmulders (via GitHub)" <gi...@apache.org>.
mthmulders commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1192132236


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);
+
             project.addAttachedArtifact(new ConsumerPomArtifact(project, generatedFile, session));
         } else if (project.getModel().isRoot()) {
             throw new IllegalStateException(
                     "The use of the root attribute on the model requires the buildconsumer feature to be active");
         }
     }
 
+    private void removeOldConsumerPomFiles(Path generatedFile) throws IOException {
+        List<Path> oldConsumerPomFiles;

Review Comment:
   That makes perfect sense to me. Thanks for commenting on 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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] gnodet closed pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet closed pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir
URL: https://github.com/apache/maven/pull/1105


-- 
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


[GitHub] [maven] Giovds commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "Giovds (via GitHub)" <gi...@apache.org>.
Giovds commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1191651779


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);
+
             project.addAttachedArtifact(new ConsumerPomArtifact(project, generatedFile, session));
         } else if (project.getModel().isRoot()) {
             throw new IllegalStateException(
                     "The use of the root attribute on the model requires the buildconsumer feature to be active");
         }
     }
 
+    private void removeOldConsumerPomFiles(Path generatedFile) throws IOException {
+        List<Path> oldConsumerPomFiles;

Review Comment:
   I had a couple of reasons for it. The first reason for me is readability (as I'll show down below). 
   
   The second is that I don't think its a good practice to wrap the checked exception into a runtime exception (also introducing more code or a wrapper 'delete' method). Especially since `injectTransformedArtifacts()` already throws a checked exception which is probably accounted for somewhere at some point.
   
   As far as I know when you rewrite the stream to something like `stream.filter(path -> /*filter*/ }).forEach(Files::delete);` you either have to wrap the checked exception thrown by `Files.delete(path)` into a runtime exception:
   ```java
       private void delete( Path path ) {
           try { Files.delete( path ); }
           catch ( IOException e ) { throw new RuntimeException("", e); }
       }
       
       /* obfuscated*/
       
       stream.filter(path -> /*filter*/ })
              .forEach(this::delete);
   ```
   Or handle it:
   ```java
   stream.filter(path -> /*filter*/ })
              .forEach(path -> {
                   try { Files.delete(path); }
                   catch (IOException ex) { /*handle the exception*/ }
              });
   ```



-- 
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


[GitHub] [maven] gnodet commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1194812745


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);

Review Comment:
   Can we move the call to `removeOldConsumerPomFiles` before generating the new file and remove them all instead of having to pass the newly created file as an argument ?



##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);

Review Comment:
   Also, I think this removing *all* matching files from the temporary directory is problematic.  If we have two concurrent builds running, one of the files will be lost.  This may be done only when `buildDirectory != null`.



-- 
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


[GitHub] [maven] mthmulders commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "mthmulders (via GitHub)" <gi...@apache.org>.
mthmulders commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1191343803


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);
+
             project.addAttachedArtifact(new ConsumerPomArtifact(project, generatedFile, session));
         } else if (project.getModel().isRoot()) {
             throw new IllegalStateException(
                     "The use of the root attribute on the model requires the buildconsumer feature to be active");
         }
     }
 
+    private void removeOldConsumerPomFiles(Path generatedFile) throws IOException {
+        List<Path> oldConsumerPomFiles;

Review Comment:
   Why does the code collect all the files in a list if it could as well remove them in the terminal operation of the `j.u.s.Stream`?



-- 
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


[GitHub] [maven] Giovds commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "Giovds (via GitHub)" <gi...@apache.org>.
Giovds commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1195574336


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);

Review Comment:
   > I think it's easier to remove all files, then generate the new one...
   
   Fair point. It makes it simpler and clearer, nice catch!
   
   > Also, I think this removing _all_ matching files from the temporary directory is problematic. If we have two concurrent builds running, one of the files will be lost. This may be done only when `buildDirectory != null`.
   
   It would be a problem. I don't think we can safely delete any files in that case without introducing too much complexity, so the behaviour would be to just let the files live in the temp dir and _maybe_ get cleaned up by the OS. Some do and some don't.
   
   I didn't know the temp dir would/could be used as build directory (I just looked and the files were indeed there). Out of curiosity; when would this be the case?



-- 
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


[GitHub] [maven] gnodet commented on pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1105:
URL: https://github.com/apache/maven/pull/1105#issuecomment-1557791393

   Closing this PR in favour of #1117 


-- 
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


[GitHub] [maven] gnodet commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1195672284


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);

Review Comment:
   > > Also, I think this removing _all_ matching files from the temporary directory is problematic. If we have two concurrent builds running, one of the files will be lost. This may be done only when `buildDirectory != null`.
   > 
   > It would be a problem. I don't think we can safely delete any files in that case without introducing too much complexity, so the behaviour would be to just let the files live in the temp dir and _maybe_ get cleaned up by the OS. Some do and some don't.
   
   There may be a better alternative.  I wonder if we could just add a `@PreDestroy` method that would keep a list of generated files (be it in the temp directory or in the build directory) and delete them when the bean is destroy...
   
   > I didn't know the temp dir would/could be used as build directory (I just looked and the files were indeed there). Out of curiosity; when would this be the case?
   
   Actually, I did not know either, I just noticed it while reviewing 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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [maven] Giovds commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "Giovds (via GitHub)" <gi...@apache.org>.
Giovds commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1190114383


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");

Review Comment:
   This was added in [MNG-7622](https://issues.apache.org/jira/browse/MNG-7622) at [this line](https://github.com/apache/maven/pull/907/files#diff-b0e0e02760045bbda4d9839297d968b0218de307f1c4a35a3262d8cb406e0ea3R70). The newly created files currently are extensionless and suffixed `pom` (see screenshot 1). 
   
   If I suffix the newly created files with `.pom` it will be seen as a `POM` extension in my file explorer (screenshot 2).
   Would this not be a different ticket to change that behaviour? I'm happy to change it here if not, but I'll have to check if it impacts other places.
   
   [1]
   <img width="533" alt="image" src="https://github.com/apache/maven/assets/27761321/9e64036a-ff9e-482f-9122-71412b113aa1">
   [2]
   <img width="449" alt="image" src="https://github.com/apache/maven/assets/27761321/eeb2b124-a48c-44d0-b81d-5631814a5a25">
   <img width="592" alt="image" src="https://github.com/apache/maven/assets/27761321/809f2d4a-a041-4095-8f3e-30881a36dda5">
   



-- 
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


[GitHub] [maven] Giovds commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "Giovds (via GitHub)" <gi...@apache.org>.
Giovds commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1197071123


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);

Review Comment:
   > @Giovds have a look at https://github.com/gnodet/maven/commit/11009e9f6921657e31384bd9838d5ab1edb1aacc, could you double check that it works for you ?
   
   Cool, TIL! Just verified it and it does work for me. The files are cleaned up. 
   
   Do note that the final result means there will be no consumer*pom file in the build dir whatsoever. I don't know if it was kept for a reason. Perhaps the file is/will be (re)used for build-consumer features beyond a single run? If it's not and it's a temp file, why not always use the temp dir e.g.?
   
   \* Without to much research: * Seems like (in this class at least) the `CONSUMER_POM_CLASSIFIER` is private so not used by other classes and the build dir is never queried for the file, only in-memory artifact lists?



-- 
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


[GitHub] [maven] michael-o commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1190121304


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");

Review Comment:
   That's different issue. Just noticed.



-- 
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


[GitHub] [maven] michael-o commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1190060397


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");

Review Comment:
   The suffix should be `.pom`, look at the Javadoc of this method.
   It is *not* the extension.



##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);
+
             project.addAttachedArtifact(new ConsumerPomArtifact(project, generatedFile, session));
         } else if (project.getModel().isRoot()) {
             throw new IllegalStateException(
                     "The use of the root attribute on the model requires the buildconsumer feature to be active");
         }
     }
 
+    private void removeOldConsumerPomFiles(Path generatedFile) throws IOException {
+        List<Path> oldConsumerPomFiles;
+        String newestFileName = generatedFile.getFileName().toString();
+        try (Stream<Path> stream = Files.walk(generatedFile.getParent(), 1)) {
+            oldConsumerPomFiles = stream.filter(path -> {
+                        String fileName = path.getFileName().toString();
+                        return !fileName.equals(newestFileName)
+                                && fileName.startsWith(CONSUMER_POM_CLASSIFIER)
+                                && fileName.endsWith("pom");

Review Comment:
   Ditto.



-- 
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


[GitHub] [maven] gnodet commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1195973569


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);

Review Comment:
   @Giovds have a look at https://github.com/gnodet/maven/commit/11009e9f6921657e31384bd9838d5ab1edb1aacc, could you double check that it works for you ?



-- 
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


[GitHub] [maven] gnodet commented on a diff in pull request #1105: [MNG-7740] Remove old temporary consumer*pom files from buildDir

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1105:
URL: https://github.com/apache/maven/pull/1105#discussion_r1200930937


##########
maven-core/src/main/java/org/apache/maven/internal/transformation/ConsumerPomArtifactTransformer.java:
##########
@@ -77,13 +80,34 @@ public void injectTransformedArtifacts(MavenProject project, RepositorySystemSes
                 Files.createDirectories(buildDir);
                 generatedFile = Files.createTempFile(buildDir, CONSUMER_POM_CLASSIFIER, "pom");
             }
+
+            removeOldConsumerPomFiles(generatedFile);

Review Comment:
   > > @Giovds have a look at [gnodet@11009e9](https://github.com/gnodet/maven/commit/11009e9f6921657e31384bd9838d5ab1edb1aacc), could you double check that it works for you ?
   > 
   > Cool, TIL! Just verified it and it does work for me. The files are cleaned up.
   > 
   > Do note that the final result means there will be no consumer*pom file in the build dir whatsoever. I don't know if it was kept for a reason. Perhaps the file is/will be (re)used for build-consumer features beyond a single run? If it's not and it's a temp file, why not always use the temp dir e.g.?
   > 
   > * Without to much research: * Seems like (in this class at least) the `CONSUMER_POM_CLASSIFIER` is private so not used by other classes and the build dir is never queried for the file, only in-memory artifact lists?
   
   Yes, I think it was written to disk because it was needed as a file and there was no easy way to delete it at the end of the process, but they're not supposed to outlive the session.



-- 
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