Skip to content
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

update OutputUtils to support stacktrace links leading to JDK files. #5091

Merged
merged 2 commits into from
Jan 9, 2023

Conversation

mbien
Copy link
Member

@mbien mbien commented Dec 12, 2022

fixes #4516

  • add active JDK to class path
  • fix the packageName parsing of the line action when modules are involved
  • open class file if no source found -> allows user to download/attach sources

second commit is for cleanup, first has the changes.

test snippet

    public static void main(String[] args) throws Exception {
        // for a JDK/application trace
        new FileReader("nonexistent");

        // for a trace in 3rd party deps (guava)
        ImmutableSet.of("a").iterator().remove();
    }

devbuild link (expires in 7 days):
https://github.com/apache/netbeans/suites/10002051531/artifacts/485721202

@mbien mbien added Maven [ci] enable "build tools" tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Dec 12, 2022
@mbien mbien added this to the NB17 milestone Dec 12, 2022
@mbien mbien added the kind:bug Bug report or fix label Dec 12, 2022
@mbien
Copy link
Member Author

mbien commented Dec 22, 2022

rebased to latest master

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Looks sane to me and a quick test on JDK 8 and 11 showed, that it works both with and without modules.

One thing I'm not really sure about is the order of classpaths:

       return createProxyClassPath(
                    Stream.concat(
                        Stream.of(prov.getProjectClassPaths(ClassPath.BOOT)),
                        Stream.of(prov.getProjectClassPaths(ClassPath.EXECUTE)))
                );

consider nbjavac. When it is declared as a dependency it overrides the built-in javac although normal classloading behavior would be to prefer the boot classpath. So my assumption would be, that people having dependencies clashing with JDK classes would ensure that the classloaders are setup to prefer these. But that is a niche use-case, so feel free to disagree and merge as is.

 - add active JDK to class path
 - fix the packageName parsing of the line action
 - open class file as fallback -> user may want to download source
 - prefer switch over long if chain
 - removed dead code
 - simplified loops
 - other minor improvements
@mbien
Copy link
Member Author

mbien commented Jan 9, 2023

consider nbjavac. When it is declared as a dependency it overrides the built-in javac although normal classloading behavior would be to prefer the boot classpath. So my assumption would be, that people having dependencies clashing with JDK classes would ensure that the classloaders are setup to prefer these. But that is a niche use-case, so feel free to disagree and merge as is.

@matthiasblaesing That is a good point. I thought about this before and wasn't sure either. But I think having project-cp before boot cp might be indeed the better default for stack trace linking since it covers more cases than the other way around (i think). Swapped it now and rebased to latest master for a fresh build.

@mbien
Copy link
Member Author

mbien commented Jan 9, 2023

all green. merging.

@mbien mbien removed DO NOT squash ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Jan 9, 2023
@mbien mbien merged commit f77b423 into apache:master Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code cleanup kind:bug Bug report or fix Maven [ci] enable "build tools" tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links in output window stacktraces do not work (when they point to JDK code)
5 participants