Re: RFR [9] Warning notice for types in Incubator Modules

[Magnus Ihse Bursie]

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 
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 

Re: RFR [9] Warning notice for types in Incubator Modules

[Chris Hegarty]

Thanks Erik and Magnus,

Since this is already pushed, I propose to file a new issue to
push these cleanups.

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 
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 ( 

Re: 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  
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

Re: RFR [9] Warning notice for types in Incubator Modules

[Chris Hegarty]

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  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?


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.

-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

Re: RFR [9] Warning notice for types in Incubator Modules

[Magnus Ihse Bursie]

On 2017-01-25 13:51, Chris Hegarty wrote:

On 24 Jan 2017, at 16:44, Erik Joelsson  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.


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, that's the 
wrong place to put it. If it does, perhaps a comment indicating this 
hidden behavior would be in place.


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 and we've had our share of bugs due to single $ in eval'd macros.


In the line: "docs-javadoc: $(BUILD_TOOLS_JDK) $(TARGETS)", please 
remove the reference to BUILD_TOOLS_JDK. It is not needed.


/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

Re: RFR [9] Warning notice for types in Incubator Modules

[Chris Hegarty]

> On 24 Jan 2017, at 16:44, Erik Joelsson  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/

-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
> 

Re: RFR [9] Warning notice for types in Incubator Modules

[Michael McMahon]

Hi Chris

As regards the networking changes, they look fine to me.
As discussed off list, I think the link generated by the @Incubating
tag might possibly point to a new page in the docs bundle rather than
directly to the JEP 11 page. But, that can be the subject of another change.

Thanks,

Michael.

On 24/01/17 14: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

Re: RFR [9] Warning notice for types in Incubator Modules

[Erik Joelsson]

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.


/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

RFR [9] Warning notice for types in Incubator Modules

[Chris Hegarty]

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