You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "garydgregory (via GitHub)" <gi...@apache.org> on 2023/04/29 20:17:06 UTC
[GitHub] [maven-dependency-analyzer] garydgregory opened a new pull request, #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
garydgregory opened a new pull request, #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89
[MSHARED-1248] maven-dependency-analyzer should log instead of failing when analyzing a corrupted jar file
https://issues.apache.org/jira/browse/MSHARED-1248
- [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)
--
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 #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#discussion_r1233373650
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -75,6 +75,9 @@ public void visitClass(String className, InputStream in) {
// some bug inside ASM causes an IOB exception. Log it and move on?
// this happens when the class isn't valid.
logger.warn("Unable to process: " + className);
+ } catch (IllegalArgumentException e) {
+ // [MSHARED-1248] should log instead of failing when analyzing a corrupted jar file
+ logger.warn("Unable to process: " + className, e);
Review Comment:
I'm still not sure if it will be the best solution.
Eg. when analyzer will not support jdk in newer version - user will only have a warnings but nothing will be analyzed.
We know that warnings are many times ignored ...
--
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 #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#issuecomment-1597073456
I think this PR is the better approach. Excluding files only works when you know in advance which files are corrupt. I know from experience that's not always true. There are corrupt jar files in the wild, including a few in Maven Central. The general principle in play is that tools should accept any input, including arbitrary byte sequences that do not meet expectations, and gracefully reject them without crashing. In this case that means the dependency analyzer should log the problem with a particular jar file and continue with the rest of the build.
Since the dependency analyzer is a library, not a plugin, it should never abort the build. It can report the issues it detects up the chain to plugins that can be configured to respond to a corrupt jar in the way that makes the most sense for the particular project.
--
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 #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo merged PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89
--
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 #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#discussion_r1233970781
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -75,6 +75,9 @@ public void visitClass(String className, InputStream in) {
// some bug inside ASM causes an IOB exception. Log it and move on?
// this happens when the class isn't valid.
logger.warn("Unable to process: " + className);
+ } catch (IllegalArgumentException e) {
+ // [MSHARED-1248] should log instead of failing when analyzing a corrupted jar file
+ logger.warn("Unable to process: " + className, e);
Review Comment:
Make this log message distinct. E.g. "Byte code of " + className + " is corrupt" and possibly include the name or path of the jar file in which the class appears.
--
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 #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#discussion_r1233258151
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -71,7 +71,7 @@ public void visitClass(String className, InputStream in) {
reader.accept(classVisitor, 0);
} catch (IOException exception) {
exception.printStackTrace();
- } catch (IndexOutOfBoundsException e) {
+ } catch (IndexOutOfBoundsException | IllegalArgumentException e) {
Review Comment:
The comment below doesn't apply to the new exception. It's probably better to make this a spearate catch block with a more descriptive error message.
--
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] garydgregory commented on a diff in pull request #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#discussion_r1233378937
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -75,6 +75,9 @@ public void visitClass(String className, InputStream in) {
// some bug inside ASM causes an IOB exception. Log it and move on?
// this happens when the class isn't valid.
logger.warn("Unable to process: " + className);
+ } catch (IllegalArgumentException e) {
+ // [MSHARED-1248] should log instead of failing when analyzing a corrupted jar file
+ logger.warn("Unable to process: " + className, e);
Review Comment:
Hi @slawekjaranowski
I'm not sure how else you propose to solve https://issues.apache.org/jira/browse/MSHARED-1248
Ideas?
--
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] garydgregory commented on a diff in pull request #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#discussion_r1234261257
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -75,6 +75,9 @@ public void visitClass(String className, InputStream in) {
// some bug inside ASM causes an IOB exception. Log it and move on?
// this happens when the class isn't valid.
logger.warn("Unable to process: " + className);
Review Comment:
@elharo
I update the call.
--
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] garydgregory commented on a diff in pull request #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#discussion_r1234261257
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -75,6 +75,9 @@ public void visitClass(String className, InputStream in) {
// some bug inside ASM causes an IOB exception. Log it and move on?
// this happens when the class isn't valid.
logger.warn("Unable to process: " + className);
Review Comment:
@elharo
I updated the call.
--
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 #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#discussion_r1233971298
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -75,6 +75,9 @@ public void visitClass(String className, InputStream in) {
// some bug inside ASM causes an IOB exception. Log it and move on?
// this happens when the class isn't valid.
logger.warn("Unable to process: " + className);
Review Comment:
Might be helpful to include the exception in the warning here as well
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -75,6 +75,9 @@ public void visitClass(String className, InputStream in) {
// some bug inside ASM causes an IOB exception. Log it and move on?
// this happens when the class isn't valid.
logger.warn("Unable to process: " + className);
+ } catch (IllegalArgumentException e) {
+ // [MSHARED-1248] should log instead of failing when analyzing a corrupted jar file
+ logger.warn("Unable to process: " + className, e);
Review Comment:
Make this log message distinct. E.g. "Byte code of " + className + " is corrupt" an possibly include the name or path of the jar file in which the class appears.
--
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] garydgregory commented on pull request #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#issuecomment-1596286870
@slawekjaranowski
That sounds reasonable but much more complicated, which I am ok with as long the use case in the jira ticket can be handled. So I guess you can close this ticket but I am not sure who will do this work.
--
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] garydgregory commented on a diff in pull request #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#discussion_r1233317327
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -71,7 +71,7 @@ public void visitClass(String className, InputStream in) {
reader.accept(classVisitor, 0);
} catch (IOException exception) {
exception.printStackTrace();
- } catch (IndexOutOfBoundsException e) {
+ } catch (IndexOutOfBoundsException | IllegalArgumentException e) {
Review Comment:
Hi @elharo
I rewrote the catch clause per your suggestion.
--
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 #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#discussion_r1233382350
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -75,6 +75,9 @@ public void visitClass(String className, InputStream in) {
// some bug inside ASM causes an IOB exception. Log it and move on?
// this happens when the class isn't valid.
logger.warn("Unable to process: " + className);
+ } catch (IllegalArgumentException e) {
+ // [MSHARED-1248] should log instead of failing when analyzing a corrupted jar file
+ logger.warn("Unable to process: " + className, e);
Review Comment:
As I see all project production and tests classes are analyzed.
Maybe allowing exclusion for some path, patterns will be what we need.
In specific project we know which classes are broken and which should be excluded.
So we not need to ignore errors when we will can exclude what we want.
@garydgregory - What do you think?
--
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] garydgregory commented on a diff in pull request #89: [MSHARED-1248] maven-dependency-analyzer should log instead of failing
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #89:
URL: https://github.com/apache/maven-dependency-analyzer/pull/89#discussion_r1234261733
##########
src/main/java/org/apache/maven/shared/dependency/analyzer/asm/DependencyClassFileVisitor.java:
##########
@@ -75,6 +75,9 @@ public void visitClass(String className, InputStream in) {
// some bug inside ASM causes an IOB exception. Log it and move on?
// this happens when the class isn't valid.
logger.warn("Unable to process: " + className);
+ } catch (IllegalArgumentException e) {
+ // [MSHARED-1248] should log instead of failing when analyzing a corrupted jar file
+ logger.warn("Unable to process: " + className, e);
Review Comment:
@elharo
I updated the message. I do not see where to get a path since we are starting from an InputStream.
--
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