gradle / gradle

Adaptable, fast automation for all
https://gradle.org
Apache License 2.0
16.9k stars 4.73k forks source link

Javadoc tasks send standard-doclet arguments to custom doclets #11898

Open AlexLandau opened 4 years ago

AlexLandau commented 4 years ago

I maintain a build that has a Javadocs task using a custom doclet (specifically, https://github.com/MarkusBernhardt/xml-doclet). This build broke when upgrading Gradle from 5.6.4 to 6.0.1; the javadoc executable gave an error message javadoc: error - invalid flag: -notimestamp.

I'm not very familiar with the Javadocs ecosystem, but it seems -notimestamp is a flag of the standard doclet, not the core executable. I would expect it to not get sent if a custom doclet is used.

The Gradle code does already include this distinction (between javadoc arguments and standard-doclet arguments) in its structure, having both a StandardJavadocDocletOptions and a CoreJavadocOptions.

Expected Behavior

When a Javadoc task uses a custom doclet, don't send arguments that are specific to the standard doclet (namely, the -doctitle and -notimestamp arguments). Probably make an exception for anything manually specified.

Current Behavior

The -doctitle and -notimestamp flags are sent regardless of which doclet is used, sometimes causing javadoc errors.

Context

I can work around the issue by adding the following lines to the task config:

task buildXmlJavadoc(type: Javadoc) {
    // ...
    title = ''
    options.noTimestamp(false)
    // ...
}

Mostly the impact is that this was an unexpected failure when trying to upgrade, and there may be more instances of that in the future. (It also took a while to figure out the workaround.)

Steps to Reproduce

Repro here: https://github.com/AlexLandau/gradle-javadoc-notimestamp-error Note that this specific repro needs to be run with Java 8. I used Azul: https://www.azul.com/downloads/zulu-community/

Your Environment

Run on MacOS with Java 8.

AlexLandau commented 4 years ago

Additional note: I don't know to what extent other custom doclets may use the -doctitle and -notimestamp flags (particularly -doctitle, which was set in versions before 6.0). If they do, the behavior change I'm suggesting would change the javadoc behavior for them.

dweiss commented 4 years ago

It's a serious regression for people using custom doclets. This commit is responsible for adding this: https://github.com/gradle/gradle/commit/9a2ee4817c30

@vlsi You added it - could you take a look?

vlsi commented 4 years ago

Just in case, the current Gradle implementation is as follows: 1) Gradle always uses StandardJavadocDocletOptions class for the options no matter which doclet is used 2) Gradle builds all the options to the options file, and it launches javadoc with that Of course, if the custom doclet is used, it might easily crash if an unexpected option is added to the file.

Note: doclet API does have a way to check if an option is supported at all or not. That is optionLength method of the doclet

Note: it would require to load the doclet class in order to call optionLength. So there's no clear way to check if the option is supported or not. I don't think there's an API to tell the doclet to ignore unsupported options.

So I see the following options: A) Just leave it as is and document the behavior (e.g. add warning at the execution time). That is, leave the configuration up to the users who configure custom doclets B) Implement optionLength-based detection of unsupported options and do something if the option is not supported by the doclet C) File a ticket to JavaDoc so they switch to notimestamp by default (I don't think it is feasible :( )

Note: I do think that the exclusion of timestamps by default helps a lot. There is extreme amount of projects that just configure javadoc, and they tend to produce gigabytes of timestamp-induced diffs.

I do see you have issues, and I recognize that is sad, however, I don't run in those issues, and I don't feel I want to implement optionLength-based approach.

Gradle's model for javadoc task is not ideal, however, it works, and workarounds are clearly available. So I don't think the issue must be a priority.

However, feel free to file a PR, and I might be happy to review it.

dweiss commented 4 years ago

Well, this check could be moved to javadoc task: if no custom doclet is specified then you could emit notimestamp (if it doesn't have any value other than the default, unset state).

An alternative would be to allow javadoc task users to override the options with a different subclass than StandardJavadocDocletOptions. The field already has a "minimal" type anyway so I don't see the reason why StandardJavadocDocletOptions should be the enforced (unchangeable) default.

vlsi commented 4 years ago

if no custom doclet is specified then you could emit notimestamp

That would be a non-tivial change, and it would result in different behavior depending on the doclet.

Suppose there's a user with a custom doclet that does support notimestamp option. What would you suggest?

That is why I don't consider that approach.

StandardJavadocDocletOptions should be the enforced (unchangeable) default.

AFAIK that is the current implementation. Of course, it might be smarter, however, that implementation requires effort to implement, and it would require some effort from the maintainers to review the change.

vlsi commented 4 years ago

(unchangeable) default

I guess it is very well changable. Have you tried tasks.withType<Javadoc>().configureEach { ... } ?

I think it should be able to override the options.

Note: current JavaDoc implementation does not support lazy configuration resolution for custom doclets. So I would expect that the users of custom doclets would anyway use their workarounds there

For instance, here's what I have to activate lazy resolution (e.g. to avoid doclet resolution when I build regular jar only): https://github.com/autostyle/autostyle/blob/3edcb79a2ddd0c8e6a988d5b6d8fd3ee2f9a729a/buildSrc/src/main/kotlin/mdoclet.gradle.kts#L41-L45

I don't think adding notimestamp there would hurt much.

dweiss commented 4 years ago

Well, you have already changed the default and this changed the behavior for everyone... Going back would revert the previous default (and javadoc tool default). You're force-feeding an option to javadoc that normally isn't there.

As for configuring javadoc, the options field's type isn't configurable. https://github.com/gradle/gradle/blob/8df8e9b6746d7f8e7d34b802719e28754bdaa056/subprojects/language-java/src/main/java/org/gradle/api/tasks/javadoc/Javadoc.java#L100

vlsi commented 4 years ago

behavior for everyone

Do you have estimates on the number of impacted projects? Do you have estimates on the number of projects that use custom doclets?

My understanding is that the number of projects/builds with custom doclets is much less that the number of total projects and builds that use javadoc. That is why I suppose that the removal of timestamps helps a lot of Gradle users.

The workaround is there, so I don't see why there must be a rush.

If you feel that javadoc task with custom doclets is important, please feel free to file a PR.

Frankly speaking, I don't have enough time and passion to improve that.

As for configuring javadoc, the options field's type isn't configurable.

I mean you can have the following workaround:

options.noTimestamp(false)

Note: if you support a custom doclet, then you might want to release a Gradle plugin that would configure javadoc task to use the doclet. The plugin could configure the relevant options as well.

dweiss commented 4 years ago

Vladimir, you changed the default of each and every javadoc task invocation (unless timestamp option was explicitly set): previously it didn't add "-notimestamp" option to javadoc invocation, since your commit it does.

I understand the intention was to improve the "default" look and feel of generated documentation but I don't think this is the build tool's job to do so. It's "javadoc" not "javadocWithNiceDefaults". OpenJDK's javadoc-mailing list is the right place to try to change the tool defaults; downstream build tools like ant or gradle shouldn't just pick a set of options they deem "right" for people. Imagine a surprise when you upgrade the build tool and all of a sudden your outputs are different (on the same JDK).

I know there is a workaround but you could argue the exact opposite: if somebody doesn't like the default they can just as easily modify the defaults of all javadoc tasks in their projects with a single statement...

bq. Frankly speaking, I don't have enough time and passion to improve that.

My whole point is that it's not about improving the state of things. It's about reverting a perceived "improvement" that in my opinion isn't one. Javadoc task should behave the same between gradle updates on the same underlying JDK (because your expect the same set of options to be passed to the tool).

If you disagree, fine. I just expressed my opinion on the matter.

t-beckmann commented 4 years ago

I agree with @dweiss, just wasted half an hour on finding this issue, added option.noTimestamp(false) as well.

shoaniki commented 3 years ago

This is even worse in the Kotlin DSL, where the noTimestamp option can't be accessed without a static typecast. option.noTimestamp(false) thus becomes the rather cumbersome (options as StandardJavadocDocletOptions).noTimestamp(false). This is not very discoverable, particularly if you don't know that's what you're looking for!

The Maven javadoc plugin solves this by simply providing a property useStandardDocletOptions, which defaults to true, but can be set to false when using a custom doclet. This seems like a sensible compromise: the defaults can be whatever you judge best for most users, but there is a straightforward, obvious, and future-proof way to turn off the magic if it breaks things.

lbergelson commented 2 years ago

I'm glad this discussion exists or I would have spent a long time trying to figure out where that option was coming from. I had assumed it was a misconfiguration in our complicated custom doclet invocations that was somehow exposed on upgrade. I didn't expect it to be a new "feature" in 6.x. This is an unintuitive change.

It is mentioned in the upgrade docs but I didn't notice that until after I understood what the problem was. Adding a line to the upgrade docs about disabling the change with options.noTimestamp(false) for custom doclets would be helpful.

manticore-projects commented 1 year ago

Greetings.

Apologies for coming late to the party. I have taken up the XML Doclet and amended it: 1) working around the missing options title and noTimestamp 2) works with any modern JDK as long as languageVersion.set(JavaLanguageVersion.of(11)) is set 3) outputs XML, Sphinx Restructured Text (and planned support for Markdown, Docbook and Ascii Doctor)

So the workarounds are not necessary anymore. Cheers!