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

JDK-8267204: Expose access to underlying streams in Reporter #4077

Conversation

jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented May 18, 2021

Please review a relatively simple update to expose the 1 or 2 streams available within javadoc for writing expected or diagnostic output.

The change at this time is just to expose the streams; followup work may change how we use them.

There are two points of note:

  • JavadocTester is extended to provided access to the javadoc entry point used by the launcher. This is simpler than exec-ing the tool to invoke the entry point, and allows us to use the existing code to capture and analyze the output.
  • The initialization of the default streams in Messager is deferred, to allow Messager to use the values of System.out and System.err at the time that javadoc is started, instead of when the class was loaded. This is part of the problems described separately in JDK-8267203, but which needs to be addressed here as part of this work.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8267204: Expose access to underlying streams in DocletEnvironment

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4077/head:pull/4077
$ git checkout pull/4077

Update a local copy of the PR:
$ git checkout pull/4077
$ git pull https://git.openjdk.java.net/jdk pull/4077/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4077

View PR using the GUI difftool:
$ git pr show -t 4077

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4077.diff

@bridgekeeper
Copy link

bridgekeeper bot commented May 18, 2021

👋 Welcome back jjg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label May 18, 2021
@openjdk
Copy link

openjdk bot commented May 18, 2021

@jonathan-gibbons The following label will be automatically applied to this pull request:

  • javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the javadoc javadoc-dev@openjdk.org label May 18, 2021
@mlbridge
Copy link

mlbridge bot commented May 18, 2021

Webrevs

@pavelrappo
Copy link
Member

I have two high-level questions: (1) Why would Doclet API expose standard streams given that it already has jdk.javadoc.doclet.Reporter? (2) Is PrintStream the most appropriate abstraction for a standard stream? (For example, the javax.tools.Tool API uses OutputStream for that purpose.)

@jonathan-gibbons
Copy link
Contributor Author

I have two high-level questions: (1) Why would Doclet API expose standard streams given that it already has jdk.javadoc.doclet.Reporter? (2) Is PrintStream the most appropriate abstraction for a standard stream? (For example, the javax.tools.Tool API uses OutputStream for that purpose.)

  1. Reporter says nothing about streams. javadoc is inconsistent with javac and when run with two streams, it sends warnings and errors to one stream and notes to the other. javac now sends all diagnostics to the error stream, leaving stdout for command-line help etc. This PR is part 1 of 2; having made both streams explicitly available here, part 2 will align javadoc with javac. Also, DocletEnvironment seemed a more appropriate place to expose the streams.

  2. For passing in streams/writers, higher level types, like OutputStream and Writer, are OK, because they can be wrapped into the more convenient forms. For passing out streams/writers, you want to pass out the Print--- forms, to save the user having to wrap and flush and close them.

@pavelrappo
Copy link
Member

  1. Reporter says nothing about streams. javadoc is inconsistent with javac and when run with two streams, it sends warnings and errors to one stream and notes to the other. javac now sends all diagnostics to the error stream, leaving stdout for command-line help etc. This PR is part 1 of 2; having made both streams explicitly available here, part 2 will align javadoc with javac. Also, DocletEnvironment seemed a more appropriate place to expose the streams.

I can see that Reporter does not mention streams. But should it mention them given that Reporter is a higher-level abstraction? If a particular mapping between diagnostic kinds and standard streams is desired can it be achieved by configuring Reporter out of band?

What would be the best code locations to look at to get familiar with how javac works with standard streams, diagnostic output, and the like?

Don't get me wrong, by asking these questions I'm trying to better understand the intent and the design.

@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented May 19, 2021

What would be the best code locations to look at to get familiar with how javac works with standard streams, diagnostic output, and the like?

Look at Log.java, and the introduction of WriterKind.STDOUT, STDERR, and the use thereof.

Before there was WriterKind.STDOUT, STDERR, javac had the same trouble that remains in javadoc ...

  1. how to map diagnostics to streams?
  2. how to handle command-line help and other "expected" output like from -Xprint ?

In javac then, and still in javadoc today, it was hampered by the use of public single-stream constructors in the Main class. javac actually had it "worse", in that at one point it completely flipped from using System.out (only) to using System.err (only). (Sort of like the Earth's magnetic poles flipping!) There is still code in jtreg to handle that flip, although the flip occurred in versions of JDK that are long unsupported.

Part of the realization of the solution came in the JDK 9 timeframe, when someone (I won't name names) came up with the simple rule that "expected" output (like command-line help) should go to stdout/System.out so that it can be piped into other tools like grep etc. All "other"/"unexpected" output should go to stderr/System.err. That's when we expanded the set of WriterKind in Log from 3 to 5, and providing access to both kinds of output stream. Note, this does not mean 5 streams; it just means a mapping from 5 reasons to the one of the available streams.

Before: ERROR, WARN, NOTE After: ERROR, WARN, NOTE, STDOUT, STDERR

So, depending on why you wanted a stream, you could get the right one. At one point, IIRC, javadoc allowed you to specify 3 streams (!!) for error, warning, notes, but generally, we settled on 1 or 2 streams, representing System.out and System.err and aligned per the tool's choice for the diagnostic kinds. javac went "all-in" for "all diagnostics to STDERR and command-line help, etc to STDOUT. javadoc was going through its own turmoil at the time (new doclet + HTML5) and so we left javadoc alone, supported by some deprecated constructors in Log, to give javadoc what it needs. So, that's generally the history of javac and javadoc streams.

I'm looking to eventually align javadoc with javac: all diagnostics go to STDERR and direct access to STDOUT(in particular) when that is appropriate. That will be a separate commit/PR, with a tiny incompatibility that most folk will not notice. It will mean that diagnostics NOTEs written from a doclet when javadoc is run from the command line will then appear on STDERR instead of STDOUT as now. For the standard doclet, we will update it so that all those tracing messages (like Generating .... file.html) will be written directly to STDOUT and not written with Reporter.printNote, so the mainstream use of the standard doclet will not be affected. But, all that change will be in a follow-up PR. For now I'm not changing anything in any incompatible way. This PR is just about exposing the necessary underlying streams.
And, I still believe the DocletEnvironment is the right place to make the streams available; not the Reporter interface.

Note, I use terms like System.out and System.err but think of them as "conceptual terms, and not always the exact same-named streams. Sometimes, alternative streams for the same conceptual purpose can be passed in to constructors etc.

Comment on lines +154 to +156
* The implementation provided by the `javadoc` tool to
* {@link Doclet#init(Locale, Reporter) initialize doclets}
* always returns a non-{@code null} value.
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean {@link Doclet#run(DocletEnvironment) run doclets} instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I guess I did. Good catch.

Copy link
Member

@pavelrappo pavelrappo left a comment

Choose a reason for hiding this comment

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

Update copyright years.

Comment on lines +174 to +176
* The implementation provided by the `javadoc` tool to
* {@link Doclet#init(Locale, Reporter) initialize doclets}
* always returns a non-{@code null} value.
Copy link
Member

Choose a reason for hiding this comment

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

Same as two preceding comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as preceding response!

*
* @implSpec
* This implementation returns {@code null}.
* The implementation provided by the `javadoc` tool to
Copy link
Member

Choose a reason for hiding this comment

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

Use {@code javadoc} or just javadoc instead of `javadoc` (back ticks).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops

@pavelrappo
Copy link
Member

This PR has been superseded by #4216.

@jonathan-gibbons jonathan-gibbons changed the title JDK-8267204: Expose access to underlying streams in DocletEnvironment JDK-8267204: Expose access to underlying streams in Reporter Jun 4, 2021
@jonathan-gibbons jonathan-gibbons deleted the jdk-8267204.docenv-streams branch October 20, 2021 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javadoc javadoc-dev@openjdk.org rfr Pull request is ready for review
2 participants