[MSHARED-1248] maven-dependency-analyzer should log instead of failing #89
Conversation
| @@ -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) { | |||
There was a problem hiding this 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.
There was a problem hiding this comment.
Hi @elharo
I rewrote the catch clause per your suggestion.
when analyzing a corrupted jar file
garydgregory
force-pushed
the
MSHARED-1248_gdg
branch
from
524cd37
to
70dc923
Compare
| // [MSHARED-1248] should log instead of failing when analyzing a corrupted jar file | ||
| logger.warn("Unable to process: " + className, e); |
There was a problem hiding this 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 ...
There was a problem hiding this comment.
Hi @slawekjaranowski
I'm not sure how else you propose to solve https://issues.apache.org/jira/browse/MSHARED-1248
Ideas?
There was a problem hiding this 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?
|
@slawekjaranowski |
|
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. |
| @@ -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); | |||
There was a problem hiding this 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.
There was a problem hiding this comment.
@elharo
I updated the message. I do not see where to get a path since we are starting from an InputStream.
| @@ -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); | |||
There was a problem hiding this comment.
Might be helpful to include the exception in the warning here as well
There was a problem hiding this comment.
@elharo
I updated the call.
garydgregory
force-pushed
the
MSHARED-1248_gdg
branch
from
b7983ed
to
fb5f5a1
Compare
garydgregory
force-pushed
the
MSHARED-1248_gdg
branch
from
fb5f5a1
to
6ef1964
Compare
[MSHARED-1248] maven-dependency-analyzer should log instead of failing when analyzing a corrupted jar file
https://issues.apache.org/jira/browse/MSHARED-1248