Skip to content

[MJAVADOC-694] Avoid empty warn message from getResolvePathResult #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 20, 2021

Conversation

slawekjaranowski
Copy link
Member

Because user can't do nothing with such message, and exception don't break execution,
logging in verbose mode with stacktrace will be enough.


Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MJAVADOC-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MJAVADOC-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify -Prun-its to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@slawekjaranowski
Copy link
Member Author

@rfscholte please look, you last fix plugin fo java modules - MJAVADOC-568

{
cause = cause.getCause();
getLog().debug( "resole path for: " + artifactFile + " cause error: " + e.getMessage(), e );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type in resove (missing the L)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't write the stacktrace, it is not useful. If the message is empty, it must be fixed in plexus-java.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation in plexus-java for jdk < 9 is:

https://github.com/codehaus-plexus/plexus-languages/blob/e1450e99c53e45a4935f4bd65b4349877d6cdbb1/plexus-java/src/main/java/org/codehaus/plexus/languages/java/jpms/CmdModuleNameExtractor.java#L44-L47

What is your proposition for code here?

we can provide some message for UnsupportedOperationException in plexus-java ... but logging warn here is too talkative, it look like standard flow for detecting modules support

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that if there's no module descriptor nor Manifest file with an Automatic-Module-Name, you need Java 9 code to get the module name based on the filename. So you must set in JdkHome in the request if you're not running with Java 9+.
The plugin is responsible to set this up correctly.
IIRC due to a recent change this method became exposed, otherwise it would never occur. So better to improve the message of the exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably it is another issue that plugin don't set JdkHome .. all test pass so such behavior is not coverage by regression ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JdkHome should only be set when using ToolChain

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems more complicated, i don't see any test for toolchains in project, so it should be done as separate bigger issue.

cause = cause.getCause();
}

getLog().debug( "resolve path for: " + artifactFile + " cause error: " + e );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discovered an existing bug here: e should be cause

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't exactly understood your comments, do you want change message of .debug("...")?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose it to log the message of the root cause, so it should be
getLog().debug( "resolve path for: " + artifactFile + " cause error: " + cause );

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, fixed

Copy link
Contributor

@rfscholte rfscholte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give it a try; I hope that if javadoc fails, that exception is clear enough to solve the issue.

@slawekjaranowski
Copy link
Member Author

Thanks for approval, so please merge and process jira issue

@slawekjaranowski slawekjaranowski merged commit b51c5d8 into apache:master Dec 20, 2021
@slawekjaranowski slawekjaranowski deleted the MJAVADOC-694 branch December 20, 2021 20:59
@cowwoc
Copy link

cowwoc commented Jan 17, 2022

@rfscholte I don't think that <jdkToolchain> works as expected. I filed https://issues.apache.org/jira/browse/MJAVADOC-704 related to this issue.

@jira-importer
Copy link
Collaborator

Resolve #1070

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants