RFR [9] Warning notice for types in Incubator Modules

Fri Feb 10 13:50:53 UTC 2017

On 2017-01-30 17:20, Chris Hegarty wrote:
> Thanks Erik and Magnus,
>
> Since this is already pushed, I propose to file a new issue to
> push these cleanups.

Sounds good. Please do that.

/Magnus

>
> diff --git a/make/Javadoc.gmk b/make/Javadoc.gmk
> --- a/make/Javadoc.gmk
> +++ b/make/Javadoc.gmk
> @@ -314,7 +314,7 @@
>    $1_INDEX_FILE := 
> $$(JAVADOC_OUTPUTDIR)/$$($1_OUTPUT_DIRNAME)/index.html
>
>    # Rule for actually running javadoc
> -  $$($1_INDEX_FILE): $(BUILD_TOOLS_JDK) $$($1_VARDEPS_FILE) 
> $$($1_PACKAGE_DEPS) $$($1_DEPS)
> +  $$($1_INDEX_FILE): $$(BUILD_TOOLS_JDK) $$($1_VARDEPS_FILE) 
> $$($1_PACKAGE_DEPS) $$($1_DEPS)
>      $$(call LogWarn, Generating Javadoc from $$(words 
> $$($1_PACKAGES)) package(s) for $$($1_OUTPUT_DIRNAME))
>      $$(call MakeDir, $$(@D))
>          ifneq ($$($1_PACKAGES_FILE), )
> @@ -743,7 +743,7 @@
>
>
> ################################################################################ 
>
>
> -docs-javadoc: $(BUILD_TOOLS_JDK) $(TARGETS)
> +docs-javadoc: $(TARGETS)
>
>  docs-copy: $(COPY_TARGETS)
>
>
> -Chris.
>
> On 30/01/17 11:29, Erik Joelsson wrote:
>> 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
>>>>
>>