You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "elharo (via GitHub)" <gi...@apache.org> on 2023/03/11 15:41:07 UTC
[GitHub] [maven-dependency-analyzer] elharo opened a new pull request, #81: Prefer JDK classes to Plexus utils
elharo opened a new pull request, #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81
This completely removes Plexus utils from this package.
--
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-dependency-analyzer] elharo commented on a diff in pull request #81: Prefer JDK classes to Plexus utils
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1141349835
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -108,41 +106,30 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
private static void acceptDirectory( File directory, ClassFileVisitor visitor )
throws IOException
{
- if ( !directory.isDirectory() )
+
+ List<Path> classFiles = Files.walk( directory.toPath() )
+ .filter( path -> path.toFile().getName().endsWith( ".class" ) )
Review Comment:
Six of one, half dozen of another. I looked at that and saw that getFileName returned a Path instead of a String, which seemed wrong to me. But if you prefer I can change 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-dependency-analyzer] elharo merged pull request #81: [MSHARED-1224] Prefer JDK classes to Plexus utils
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo merged PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81
--
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-dependency-analyzer] michael-o commented on a diff in pull request #81: Prefer JDK classes to Plexus utils
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1141357842
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -108,41 +106,30 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
private static void acceptDirectory( File directory, ClassFileVisitor visitor )
throws IOException
{
- if ( !directory.isDirectory() )
+
+ List<Path> classFiles = Files.walk( directory.toPath() )
+ .filter( path -> path.toFile().getName().endsWith( ".class" ) )
Review Comment:
Let's make consistently use NIO2.
--
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-dependency-analyzer] elharo commented on a diff in pull request #81: Prefer JDK classes to Plexus utils
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1141458835
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -108,41 +106,30 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
private static void acceptDirectory( File directory, ClassFileVisitor visitor )
throws IOException
{
- if ( !directory.isDirectory() )
+
+ List<Path> classFiles = Files.walk( directory.toPath() )
+ .filter( path -> path.toFile().getName().endsWith( ".class" ) )
Review Comment:
might have to revert that. It seems to break the integration tests on Windows in a way that indicates a real 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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-analyzer] elharo commented on a diff in pull request #81: Prefer JDK classes to Plexus utils
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1141350483
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -108,41 +106,30 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
private static void acceptDirectory( File directory, ClassFileVisitor visitor )
throws IOException
{
- if ( !directory.isDirectory() )
+
+ List<Path> classFiles = Files.walk( directory.toPath() )
+ .filter( path -> path.toFile().getName().endsWith( ".class" ) )
+ .collect( Collectors.toList() );
+
+ for ( Path path : classFiles )
{
- throw new IllegalArgumentException( "File is not a directory" );
- }
-
- DirectoryScanner scanner = new DirectoryScanner();
-
- scanner.setBasedir( directory );
- scanner.setIncludes( new String[] { "**/*.class" } );
-
- scanner.scan();
-
- String[] paths = scanner.getIncludedFiles();
-
- for ( String path : paths )
- {
- path = path.replace( File.separatorChar, '/' );
-
- File file = new File( directory, path );
-
- try ( InputStream in = new FileInputStream( file ) )
+ try ( InputStream in = Files.newInputStream( path ) )
{
- visitClass( path, in, visitor );
+ visitClass( directory, path, in, visitor );
}
}
}
- private static void visitClass( String path, InputStream in, ClassFileVisitor visitor )
+ private static void visitClass( File baseDirectory, Path path, InputStream in, ClassFileVisitor visitor )
{
- if ( !path.endsWith( ".class" ) )
- {
- throw new IllegalArgumentException( "Path is not a class" );
- }
+ // getPath() returns a String, not a java.nio.file.Path
+ String stringPath = path.toFile().getPath().substring( baseDirectory.getPath().length() + 1 );
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-analyzer] elharo commented on a diff in pull request #81: Prefer JDK classes to Plexus utils
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1141458645
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -108,41 +106,30 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
private static void acceptDirectory( File directory, ClassFileVisitor visitor )
throws IOException
{
- if ( !directory.isDirectory() )
+
+ List<Path> classFiles = Files.walk( directory.toPath() )
+ .filter( path -> path.toFile().getName().endsWith( ".class" ) )
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-analyzer] elharo commented on a diff in pull request #81: Prefer JDK classes to Plexus utils
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1155034034
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -99,7 +97,8 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
// ignore files like package-info.class and module-info.class
if ( name.endsWith( ".class" ) && name.indexOf( '-' ) == -1 )
{
- visitClass( name, in, visitor );
+ // Even on Windows Jars use / as the separator character
Review Comment:
Yes, that's the point. The previous version was using the system dependent line separator, which was why the test was failing on Windows but not Linux.
--
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-dependency-analyzer] michael-o commented on a diff in pull request #81: Prefer JDK classes to Plexus utils
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1155001476
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -99,7 +97,8 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
// ignore files like package-info.class and module-info.class
if ( name.endsWith( ".class" ) && name.indexOf( '-' ) == -1 )
{
- visitClass( name, in, visitor );
+ // Even on Windows Jars use / as the separator character
Review Comment:
The comment is redundant and wrong because this has nothing to do with Windows. REad the PKZIP implementation note: separators are always foward slashes regarding of the OS.
--
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-dependency-analyzer] michael-o commented on a diff in pull request #81: [MSHARED-1224] Prefer JDK classes to Plexus utils
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1161016212
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -77,45 +77,39 @@ private static void acceptJar(URL url, ClassFileVisitor visitor) throws IOExcept
String name = entry.getName();
// ignore files like package-info.class and module-info.class
if (name.endsWith(".class") && name.indexOf('-') == -1) {
- visitClass(name, in, visitor);
+ // Jars(ZIP) always use / as the separator character
+ visitClass(name, in, visitor, '/');
}
}
}
}
private static void acceptDirectory(File directory, ClassFileVisitor visitor) throws IOException {
- if (!directory.isDirectory()) {
- throw new IllegalArgumentException("File is not a directory");
- }
-
- DirectoryScanner scanner = new DirectoryScanner();
-
- scanner.setBasedir(directory);
- scanner.setIncludes(new String[] {"**/*.class"});
-
- scanner.scan();
- String[] paths = scanner.getIncludedFiles();
-
- for (String path : paths) {
- path = path.replace(File.separatorChar, '/');
-
- File file = new File(directory, path);
+ List<Path> classFiles;
+ try (Stream<Path> walk = Files.walk(directory.toPath())) {
+ classFiles = walk.filter(path -> path.getFileName().toString().endsWith(".class"))
Review Comment:
There is a native `endsWith()`
--
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-dependency-analyzer] slawekjaranowski commented on a diff in pull request #81: Prefer JDK classes to Plexus utils
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1161009729
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -99,7 +97,8 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
// ignore files like package-info.class and module-info.class
if ( name.endsWith( ".class" ) && name.indexOf( '-' ) == -1 )
{
- visitClass( name, in, visitor );
+ // Even on Windows Jars use / as the separator character
Review Comment:
comment was 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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-dependency-analyzer] elharo commented on a diff in pull request #81: [MSHARED-1224] Prefer JDK classes to Plexus utils
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1161021500
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -77,45 +77,39 @@ private static void acceptJar(URL url, ClassFileVisitor visitor) throws IOExcept
String name = entry.getName();
// ignore files like package-info.class and module-info.class
if (name.endsWith(".class") && name.indexOf('-') == -1) {
- visitClass(name, in, visitor);
+ // Jars(ZIP) always use / as the separator character
+ visitClass(name, in, visitor, '/');
}
}
}
}
private static void acceptDirectory(File directory, ClassFileVisitor visitor) throws IOException {
- if (!directory.isDirectory()) {
- throw new IllegalArgumentException("File is not a directory");
- }
-
- DirectoryScanner scanner = new DirectoryScanner();
-
- scanner.setBasedir(directory);
- scanner.setIncludes(new String[] {"**/*.class"});
-
- scanner.scan();
- String[] paths = scanner.getIncludedFiles();
-
- for (String path : paths) {
- path = path.replace(File.separatorChar, '/');
-
- File file = new File(directory, path);
+ List<Path> classFiles;
+ try (Stream<Path> walk = Files.walk(directory.toPath())) {
+ classFiles = walk.filter(path -> path.getFileName().toString().endsWith(".class"))
Review Comment:
I'll have to check but I don't think that will work. The method you reference checks if a Path ends with another Path. e.g. that /usr/local/tmp/foo.class ends with a path for foo.class. However, I don't think it would work for /usr/local/tmp/foo.class ending with the extension .class.
See https://stackoverflow.com/questions/20531247/how-to-check-the-extension-of-a-java-7-path
--
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-dependency-analyzer] michael-o commented on a diff in pull request #81: [MSHARED-1224] Prefer JDK classes to Plexus utils
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1161023331
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -77,45 +77,39 @@ private static void acceptJar(URL url, ClassFileVisitor visitor) throws IOExcept
String name = entry.getName();
// ignore files like package-info.class and module-info.class
if (name.endsWith(".class") && name.indexOf('-') == -1) {
- visitClass(name, in, visitor);
+ // Jars(ZIP) always use / as the separator character
+ visitClass(name, in, visitor, '/');
}
}
}
}
private static void acceptDirectory(File directory, ClassFileVisitor visitor) throws IOException {
- if (!directory.isDirectory()) {
- throw new IllegalArgumentException("File is not a directory");
- }
-
- DirectoryScanner scanner = new DirectoryScanner();
-
- scanner.setBasedir(directory);
- scanner.setIncludes(new String[] {"**/*.class"});
-
- scanner.scan();
- String[] paths = scanner.getIncludedFiles();
-
- for (String path : paths) {
- path = path.replace(File.separatorChar, '/');
-
- File file = new File(directory, path);
+ List<Path> classFiles;
+ try (Stream<Path> walk = Files.walk(directory.toPath())) {
+ classFiles = walk.filter(path -> path.getFileName().toString().endsWith(".class"))
Review Comment:
Yes, indeed. It is just a wrapper.
--
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-dependency-analyzer] elharo commented on pull request #81: Prefer JDK classes to Plexus utils
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#issuecomment-1482771286
Trickier than I thought. The problem is that jars on Windows still use forward slash but directories use backslash.
--
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-dependency-analyzer] michael-o commented on a diff in pull request #81: Prefer JDK classes to Plexus utils
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #81:
URL: https://github.com/apache/maven-dependency-analyzer/pull/81#discussion_r1138253560
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -108,41 +106,30 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
private static void acceptDirectory( File directory, ClassFileVisitor visitor )
throws IOException
{
- if ( !directory.isDirectory() )
+
+ List<Path> classFiles = Files.walk( directory.toPath() )
+ .filter( path -> path.toFile().getName().endsWith( ".class" ) )
+ .collect( Collectors.toList() );
+
+ for ( Path path : classFiles )
{
- throw new IllegalArgumentException( "File is not a directory" );
- }
-
- DirectoryScanner scanner = new DirectoryScanner();
-
- scanner.setBasedir( directory );
- scanner.setIncludes( new String[] { "**/*.class" } );
-
- scanner.scan();
-
- String[] paths = scanner.getIncludedFiles();
-
- for ( String path : paths )
- {
- path = path.replace( File.separatorChar, '/' );
-
- File file = new File( directory, path );
-
- try ( InputStream in = new FileInputStream( file ) )
+ try ( InputStream in = Files.newInputStream( path ) )
{
- visitClass( path, in, visitor );
+ visitClass( directory, path, in, visitor );
}
}
}
- private static void visitClass( String path, InputStream in, ClassFileVisitor visitor )
+ private static void visitClass( File baseDirectory, Path path, InputStream in, ClassFileVisitor visitor )
{
- if ( !path.endsWith( ".class" ) )
- {
- throw new IllegalArgumentException( "Path is not a class" );
- }
+ // getPath() returns a String, not a java.nio.file.Path
+ String stringPath = path.toFile().getPath().substring( baseDirectory.getPath().length() + 1 );
Review Comment:
Same here
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/ClassFileVisitorUtils.java:
##########
@@ -108,41 +106,30 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
private static void acceptDirectory( File directory, ClassFileVisitor visitor )
throws IOException
{
- if ( !directory.isDirectory() )
+
+ List<Path> classFiles = Files.walk( directory.toPath() )
+ .filter( path -> path.toFile().getName().endsWith( ".class" ) )
Review Comment:
Why not `#getFileName()#toString()`?
--
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