You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/10/01 09:12:31 UTC
[GitHub] [maven-ear-plugin] zzzLobster opened a new pull request #17: [MEAR-285] Fixed failure when destination directory exists
zzzLobster opened a new pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17
[MEAR-285] Fixed failure when destination directory exists
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r498525410
##########
File path: src/main/java/org/apache/maven/plugins/ear/EarMojo.java
##########
@@ -443,7 +443,7 @@ private void copyModules( final JavaEEVersion javaEEVersion,
{
getLog().info( "Copying artifact [" + module + "] to [" + module.getUri() + "] (unpacked)" );
// Make sure that the destination is a directory to avoid plexus nasty stuff :)
- if ( !destinationFile.mkdirs() )
+ if ( !destinationFile.isDirectory() && !destinationFile.mkdirs() )
Review comment:
https://github.com/zzzLobster/maven-ear-plugin/pull/1
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r499166379
##########
File path: src/test/java/org/apache/maven/plugins/ear/it/EarMojoIT.java
##########
@@ -993,4 +993,24 @@ public void testProject087()
{
doTestProject( "project-087", new String[] { "eartest-ejb-sample-one-1.0.jar", "eartest-ejb-sample-two-1.0.jar" } );
}
+
+ /**
+ * Builds WAR and EAR as part of multi-module project twice so that the 2nd build is guaranteed to be performed when
+ * target directories and files exist.
+ * @throws Exception in case of an error.
Review comment:
Well, do you find it appropriate if I remove this useless JavaDoc from all tests in this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#issuecomment-703117231
@elharo and @hboutemy,
I assume MEAR-287 is a major bug making 3.1.0 release of Maven EAR Plugin breaking existing workflows. Could we speedup / force review of this pull request and release Maven EAR Plugin 3.1.1 ASAP?
Thank you for your work and efforts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r498525410
##########
File path: src/main/java/org/apache/maven/plugins/ear/EarMojo.java
##########
@@ -443,7 +443,7 @@ private void copyModules( final JavaEEVersion javaEEVersion,
{
getLog().info( "Copying artifact [" + module + "] to [" + module.getUri() + "] (unpacked)" );
// Make sure that the destination is a directory to avoid plexus nasty stuff :)
- if ( !destinationFile.mkdirs() )
+ if ( !destinationFile.isDirectory() && !destinationFile.mkdirs() )
Review comment:
Test created in https://github.com/zzzLobster/maven-ear-plugin/pull/1
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r499178059
##########
File path: src/test/java/org/apache/maven/plugins/ear/it/EarMojoIT.java
##########
@@ -993,4 +993,24 @@ public void testProject087()
{
doTestProject( "project-087", new String[] { "eartest-ejb-sample-one-1.0.jar", "eartest-ejb-sample-two-1.0.jar" } );
}
+
+ /**
+ * Builds WAR and EAR as part of multi-module project twice so that the 2nd build is guaranteed to be performed when
+ * target directories and files exist.
+ * @throws Exception in case of an error.
Review comment:
Removed in eeb458b
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r498934256
##########
File path: src/test/java/org/apache/maven/plugins/ear/it/EarMojoIT.java
##########
@@ -993,4 +993,24 @@ public void testProject087()
{
doTestProject( "project-087", new String[] { "eartest-ejb-sample-one-1.0.jar", "eartest-ejb-sample-two-1.0.jar" } );
}
+
+ /**
+ * Builds WAR and EAR as part of multi-module project twice so that the 2nd build is guaranteed to be performed when
+ * target directories and files exist.
+ * @throws Exception in case of an error.
Review comment:
I agree with useless of this part of this JavaDoc but I prefer to keep code consistent - all tests in this file have this `@throws` JavaDoc
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r498940815
##########
File path: src/main/java/org/apache/maven/plugins/ear/EarMojo.java
##########
@@ -749,7 +749,7 @@ private void changeManifestClasspath( EarModule module, File original, JavaEEVer
// Create a temporary work directory
// MEAR-167 use uri as directory to prevent merging of artifacts with the same artifactId
workDirectory = new File( new File( getTempFolder(), "temp" ), module.getUri() );
- if ( workDirectory.mkdirs() )
+ if ( workDirectory.isDirectory() || workDirectory.mkdirs() )
{
getLog().debug( "Created a temporary work directory: " + workDirectory.getAbsolutePath() );
Review comment:
Could we just simplify log message till:
```java
getLog().debug( "Temporary work directory: " + workDirectory.getAbsolutePath() );
```
?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] elharo commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r499159371
##########
File path: src/test/java/org/apache/maven/plugins/ear/it/EarMojoIT.java
##########
@@ -993,4 +993,24 @@ public void testProject087()
{
doTestProject( "project-087", new String[] { "eartest-ejb-sample-one-1.0.jar", "eartest-ejb-sample-two-1.0.jar" } );
}
+
+ /**
+ * Builds WAR and EAR as part of multi-module project twice so that the 2nd build is guaranteed to be performed when
+ * target directories and files exist.
+ * @throws Exception in case of an error.
Review comment:
We don't need to be consistent with mistakes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] elharo commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r498223939
##########
File path: src/main/java/org/apache/maven/plugins/ear/EarMojo.java
##########
@@ -443,7 +443,7 @@ private void copyModules( final JavaEEVersion javaEEVersion,
{
getLog().info( "Copying artifact [" + module + "] to [" + module.getUri() + "] (unpacked)" );
// Make sure that the destination is a directory to avoid plexus nasty stuff :)
- if ( !destinationFile.mkdirs() )
+ if ( !destinationFile.isDirectory() && !destinationFile.mkdirs() )
Review comment:
This needs a test. Assuming this is an issue (I can't reproduce) there are other areas that need to be fixed too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r498525410
##########
File path: src/main/java/org/apache/maven/plugins/ear/EarMojo.java
##########
@@ -443,7 +443,7 @@ private void copyModules( final JavaEEVersion javaEEVersion,
{
getLog().info( "Copying artifact [" + module + "] to [" + module.getUri() + "] (unpacked)" );
// Make sure that the destination is a directory to avoid plexus nasty stuff :)
- if ( !destinationFile.mkdirs() )
+ if ( !destinationFile.isDirectory() && !destinationFile.mkdirs() )
Review comment:
Test created in https://github.com/zzzLobster/maven-ear-plugin/pull/1. That test covers change in line 752 but not in this line. I'll check if I can extend test. Let me know if it is not required.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] elharo commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r498837830
##########
File path: src/main/java/org/apache/maven/plugins/ear/EarMojo.java
##########
@@ -749,7 +749,7 @@ private void changeManifestClasspath( EarModule module, File original, JavaEEVer
// Create a temporary work directory
// MEAR-167 use uri as directory to prevent merging of artifacts with the same artifactId
workDirectory = new File( new File( getTempFolder(), "temp" ), module.getUri() );
- if ( workDirectory.mkdirs() )
+ if ( workDirectory.isDirectory() || workDirectory.mkdirs() )
{
getLog().debug( "Created a temporary work directory: " + workDirectory.getAbsolutePath() );
Review comment:
log message is now incorrect here; probably need a nested if in this case
##########
File path: src/test/java/org/apache/maven/plugins/ear/it/EarMojoIT.java
##########
@@ -993,4 +993,24 @@ public void testProject087()
{
doTestProject( "project-087", new String[] { "eartest-ejb-sample-one-1.0.jar", "eartest-ejb-sample-two-1.0.jar" } );
}
+
+ /**
+ * Builds WAR and EAR as part of multi-module project twice so that the 2nd build is guaranteed to be performed when
+ * target directories and files exist.
+ * @throws Exception in case of an error.
Review comment:
this @throws clause isn't needed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] elharo commented on pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
elharo commented on pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#issuecomment-707083686
Trying jenkins again:
https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-ear-plugin/job/17/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] zzzLobster commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
zzzLobster commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r498712047
##########
File path: src/main/java/org/apache/maven/plugins/ear/EarMojo.java
##########
@@ -443,7 +443,7 @@ private void copyModules( final JavaEEVersion javaEEVersion,
{
getLog().info( "Copying artifact [" + module + "] to [" + module.getUri() + "] (unpacked)" );
// Make sure that the destination is a directory to avoid plexus nasty stuff :)
- if ( !destinationFile.mkdirs() )
+ if ( !destinationFile.isDirectory() && !destinationFile.mkdirs() )
Review comment:
I have merged the Marat's PR, So the PR currently contains IT for it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r498615464
##########
File path: src/main/java/org/apache/maven/plugins/ear/EarMojo.java
##########
@@ -443,7 +443,7 @@ private void copyModules( final JavaEEVersion javaEEVersion,
{
getLog().info( "Copying artifact [" + module + "] to [" + module.getUri() + "] (unpacked)" );
// Make sure that the destination is a directory to avoid plexus nasty stuff :)
- if ( !destinationFile.mkdirs() )
+ if ( !destinationFile.isDirectory() && !destinationFile.mkdirs() )
Review comment:
Hmm... it wasn't so hard to extend the test. Now it covers both changed lines, i.e. it fails if one of changes is missing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on a change in pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on a change in pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#discussion_r498953660
##########
File path: src/main/java/org/apache/maven/plugins/ear/EarMojo.java
##########
@@ -749,7 +749,7 @@ private void changeManifestClasspath( EarModule module, File original, JavaEEVer
// Create a temporary work directory
// MEAR-167 use uri as directory to prevent merging of artifacts with the same artifactId
workDirectory = new File( new File( getTempFolder(), "temp" ), module.getUri() );
- if ( workDirectory.mkdirs() )
+ if ( workDirectory.isDirectory() || workDirectory.mkdirs() )
{
getLog().debug( "Created a temporary work directory: " + workDirectory.getAbsolutePath() );
Review comment:
Logging was reworked as requested in 304c4b3
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] mabrarov commented on pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
mabrarov commented on pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17#issuecomment-706787214
What's pending / required for merging this pull request?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-ear-plugin] elharo merged pull request #17: [MEAR-287] Fixed failure when destination directory exists
Posted by GitBox <gi...@apache.org>.
elharo merged pull request #17:
URL: https://github.com/apache/maven-ear-plugin/pull/17
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org