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/05/17 21:14:53 UTC
[GitHub] [maven-shared-utils] roxspring opened a new pull request #31: [MSHARED-681] createSymbolicLink() overwrites existing different symlinks
roxspring opened a new pull request #31:
URL: https://github.com/apache/maven-shared-utils/pull/31
Inspired by @mkarg's https://github.com/apache/maven-shared/pull/12/files but:
- Reimplemented in `FileUtils.createSymbolicLink()` since the utility method was moved from `Java7Support`.
- Read the existing symlink and only delete if it's incorrectly targeted
- Added a test confirming that symlinks are correctly overwritten
----------------------------------------------------------------
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-shared-utils] roxspring commented on a change in pull request #31: [MSHARED-681] createSymbolicLink() overwrites existing different symlinks
Posted by GitBox <gi...@apache.org>.
roxspring commented on a change in pull request #31:
URL: https://github.com/apache/maven-shared-utils/pull/31#discussion_r427593386
##########
File path: src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java
##########
@@ -1475,6 +1475,30 @@ public void createAndReadSymlink()
Files.delete( file.toPath() );
}
+ @Test
+ public void createSymbolicLinkWithDifferentTargetOverwritesSymlink()
+ throws Exception
+ {
+ assumeThat( System.getProperty( "os.name" ), not( startsWith( "Windows" ) ) );
Review comment:
Unified explicit calls on `assumeFalse( Os.isFamily( Os.FAMILY_WINDOWS ) );`.
I've left the various path based assumptions as is for now.
----------------------------------------------------------------
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-shared-utils] michael-o commented on a change in pull request #31: [MSHARED-681] createSymbolicLink() overwrites existing different symlinks
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #31:
URL: https://github.com/apache/maven-shared-utils/pull/31#discussion_r427583000
##########
File path: src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java
##########
@@ -1475,6 +1475,30 @@ public void createAndReadSymlink()
Files.delete( file.toPath() );
}
+ @Test
+ public void createSymbolicLinkWithDifferentTargetOverwritesSymlink()
+ throws Exception
+ {
+ assumeThat( System.getProperty( "os.name" ), not( startsWith( "Windows" ) ) );
Review comment:
Please use the already given assume approach.
----------------------------------------------------------------
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-shared-utils] roxspring commented on a change in pull request #31: [MSHARED-681] createSymbolicLink() overwrites existing different symlinks
Posted by GitBox <gi...@apache.org>.
roxspring commented on a change in pull request #31:
URL: https://github.com/apache/maven-shared-utils/pull/31#discussion_r426311011
##########
File path: src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java
##########
@@ -1475,6 +1475,30 @@ public void createAndReadSymlink()
Files.delete( file.toPath() );
}
+ @Test
+ public void createSymbolicLinkWithDifferentTargetOverwritesSymlink()
+ throws Exception
+ {
+ assumeThat( System.getProperty( "os.name" ), not( startsWith( "Windows" ) ) );
Review comment:
An understandable concern. ~~Can the Jenkins results be seen somewhere obvious?~~ I presume it'll appear at https://builds.apache.org/job/maven-box/job/maven-shared-utils/ (it's a shame they're not linked from the PR)
FWIW the condition was copied from the symlink test immediately above, which itself was moved from `Java7SupportTest` only yesterday. Prior to that it appears to have been unchanged here for 3 years: https://github.com/apache/maven-shared-utils/blob/bb2f85e98c3c651ae50b7f642500cb74f50abb0d/src/test/java/org/apache/maven/shared/utils/io/Java7SupportTest.java#L50
On closer inspection it seems that `FileUtilsTest` is already making Windows related assumptions in a variety of ways:
- https://github.com/apache/maven-shared-utils/blob/bb2f85e98c3c651ae50b7f642500cb74f50abb0d/src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java#L446
- https://github.com/apache/maven-shared-utils/blob/92a5ba3c431affcabb2cebd11ca17fe02b15748c/src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java#L1206
- https://github.com/apache/maven-shared-utils/blob/92a5ba3c431affcabb2cebd11ca17fe02b15748c/src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java#L1461
- https://github.com/apache/maven-shared-utils/blob/92a5ba3c431affcabb2cebd11ca17fe02b15748c/src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java#L1432
If there's a preferred approach to such assumptions then I'm happy to unify on that - but perhaps in a separate 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] olamy closed pull request #31: [MSHARED-681] createSymbolicLink() overwrites existing different symlinks
Posted by GitBox <gi...@apache.org>.
olamy closed pull request #31:
URL: https://github.com/apache/maven-shared-utils/pull/31
----------------------------------------------------------------
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-shared-utils] olamy commented on pull request #31: [MSHARED-681] createSymbolicLink() overwrites existing different symlinks
Posted by GitBox <gi...@apache.org>.
olamy commented on pull request #31:
URL: https://github.com/apache/maven-shared-utils/pull/31#issuecomment-633758914
merged via 8b35ffcf1b410a0870a22ae37f18d96423d42852
----------------------------------------------------------------
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-shared-utils] roxspring commented on a change in pull request #31: [MSHARED-681] createSymbolicLink() overwrites existing different symlinks
Posted by GitBox <gi...@apache.org>.
roxspring commented on a change in pull request #31:
URL: https://github.com/apache/maven-shared-utils/pull/31#discussion_r426311011
##########
File path: src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java
##########
@@ -1475,6 +1475,30 @@ public void createAndReadSymlink()
Files.delete( file.toPath() );
}
+ @Test
+ public void createSymbolicLinkWithDifferentTargetOverwritesSymlink()
+ throws Exception
+ {
+ assumeThat( System.getProperty( "os.name" ), not( startsWith( "Windows" ) ) );
Review comment:
An understandable concern. Can the Jenkins results be seen somewhere obvious? (it's a shame they're not linked from the PR)
FWIW the condition was copied from the symlink test immediately above, which itself was moved from `Java7SupportTest` only yesterday. Prior to that it appears to have been unchanged here for 3 years: https://github.com/apache/maven-shared-utils/blob/bb2f85e98c3c651ae50b7f642500cb74f50abb0d/src/test/java/org/apache/maven/shared/utils/io/Java7SupportTest.java#L50
On closer inspection it seems that `FileUtilsTest` is already making Windows related assumptions in a variety of ways:
- https://github.com/apache/maven-shared-utils/blob/bb2f85e98c3c651ae50b7f642500cb74f50abb0d/src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java#L446
- https://github.com/apache/maven-shared-utils/blob/92a5ba3c431affcabb2cebd11ca17fe02b15748c/src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java#L1206
- https://github.com/apache/maven-shared-utils/blob/92a5ba3c431affcabb2cebd11ca17fe02b15748c/src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java#L1461
- https://github.com/apache/maven-shared-utils/blob/92a5ba3c431affcabb2cebd11ca17fe02b15748c/src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java#L1432
If there's a preferred approach to such assumptions then I'm happy to unify on that - but perhaps in a separate 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shared-utils] elharo commented on a change in pull request #31: [MSHARED-681] createSymbolicLink() overwrites existing different symlinks
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #31:
URL: https://github.com/apache/maven-shared-utils/pull/31#discussion_r426307551
##########
File path: src/test/java/org/apache/maven/shared/utils/io/FileUtilsTest.java
##########
@@ -1475,6 +1475,30 @@ public void createAndReadSymlink()
Files.delete( file.toPath() );
}
+ @Test
+ public void createSymbolicLinkWithDifferentTargetOverwritesSymlink()
+ throws Exception
+ {
+ assumeThat( System.getProperty( "os.name" ), not( startsWith( "Windows" ) ) );
Review comment:
I'm a little concerned this might not work (it doesn't in some repos for reasons) so we need to check this in Jenkins to make sure it passes on windows.
----------------------------------------------------------------
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