RFR [9] Warning notice for types in Incubator Modules

[Erik Joelsson]

Hello,

On 2017-01-30 11:58, Chris Hegarty wrote:
> Magnus,
>
> On 26/01/17 11:45, Magnus Ihse Bursie wrote:
>> On 2017-01-25 13:51, Chris Hegarty wrote:
>>>> On 24 Jan 2017, at 16:44, Erik Joelsson <erik.joelsson at oracle.com> 
>>>> wrote:
>>>>
>>>> Hello,
>>>>
>>>> Build changes look good except for one thing. In Javadoc.gmk, the 
>>>> dependency on $(BUILD_TOOLS_JDK) needs to be set for 
>>>> $$($1_INDEX_FILE) (on line 317), where the actual recipes are 
>>>> created. Setting it on the phony docs-javadoc target will not help 
>>>> incremental builds.
>>> Updated in place
>>>   http://cr.openjdk.java.net/~chegar/incubator_taglet/
>>
>> It's not ideal that a top-level target has dependencies from the JDK
>> repo, but since we have no or few pre-existing top-level tools, I
>> understand if you hesitate to add a new framework for such tools. If we
>> ever do introduce more such tools, I think we should move this along.
>
> Ok.
>
>> I think the new -taglet options to the DEFAULT_JAVADOC_OPTIONS variable.
>> As  the documentation says, the DEFAULT_JAVADOC_TAGS is there to specify
>> the ordering of tags in the output. Unless the -taglet options
>> implicitly generates one or more -tag options in it's place,
>
> It's an inline tag, so by definition has no order.
>
>> that's the
>> wrong place to put it. If it does, perhaps a comment indicating this
>> hidden behavior would be in place.
>
> It seemed best to keep all tag related options together,
> rather then spreading them across multiple variables.
>
>> In the line: "$$($1_INDEX_FILE): $(BUILD_TOOLS_JDK) $$($1_VARDEPS_FILE)
>> $$($1_PACKAGE_DEPS) $$($1_DEPS)", please use a double $ ($$) for all
>> variables. While technically not necessary in this particular case, it's
>> misleading
>
> How so?
>
>> and we've had our share of bugs due to single $ in eval'd macros.
>
> Will this specific variable cause a problem, if so how?
>
No, this variable will not cause a problem, but we have sort of agreed 
(though I have been a bit slack about it) to be consistent with all 
variable references inside macro bodies that will be eval'd. The trouble 
starts if the value of such a variable contains something that make has 
a special interpretation for, like a ':' or a ',' or a '$'. Then the 
eval will re-interpret those and weird, very hard to debug errors, start 
appearing.
>> In the line: "docs-javadoc: $(BUILD_TOOLS_JDK) $(TARGETS)", please
>> remove the reference to BUILD_TOOLS_JDK. It is not needed.
>
> Why is this not needed.
>
So we have these targets:

* docs-javadoc, which is a named phony target which we use as part of 
the external API for the Javadoc.gmk makefile.
* For each call to the SetupJavadoc macro, we have $$($1_INDEX_FILE) 
(which evaluates to something like 
build/linux-x64/images/docs/api/index.html). This is a specific file 
being generated by a recipe. All of these are added to the TARGETS 
variable through which all of them are set as prerequisites to docs-javadoc.
* The targets in $(BUILD_TOOLS_JDK) represents the files generated by 
building the tools. We add these files as prerequisites to each of the 
index files so that if the build tool has changed, the javadoc will get 
regenerated. This is what we want to achieve for proper incremental 
behavior.

Building the tools has no inherent value in itself, as they are not part 
of the product, they are only needed to generate proper javadocs. As 
such, it is conceptually wrong (as well as redundant) to also add the 
tools as prerequisites to the docs-javadoc target, because this target 
symbolizes building all the javadoc. Each of those javadoc runs do 
require the tools however, so each of those javadoc runs need to have 
the tools as prerequisites.

I had missed that you didn't remove that change in your updated review btw.

/Erik

> -Chris.
>
>> /Magnus
>>
>>
>>>
>>> -Chris.
>>>
>>>> /Erik
>>>>
>>>>
>>>> On 2017-01-24 15:08, Chris Hegarty wrote:
>>>>> As per the guidance in JEP 11 [1], the javadoc for types in
>>>>> incubator modules should have a clear and explicit warning
>>>>> notice. To that end, I’ve added a simple inline taglet that can
>>>>> be used to effectively inject a standard notice, and applied it
>>>>> to all public types in the HTTP module ( I’ll cleanup the line
>>>>> width formatting before pushing ).
>>>>>
>>>>> http://cr.openjdk.java.net/~chegar/incubator_taglet/
>>>>>
>>>>> -Chris.
>>>>>
>>>>> P.S. this work is, somewhat, in preparation for when we move
>>>>> to a single documentation bundle [2].
>>>>>
>>>>> [1] http://openjdk.java.net/jeps/11
>>>>> [2] http://openjdk.java.net/jeps/299
>>