Re: RFR: 8247957: remove doclint support for HTML 4 [v3]
On Thu, 17 Dec 2020 04:48:44 GMT, Jonathan Gibbons wrote: >> OK: valid >> OBSOLETE: obsolete, deprecated, but still supported (valid) >> UNSUPPORTED: ever supported but no longer supported (invalid) >> INVALID: the rest of others (invalid) >> >> UNSUPPORTED can be used if we would like to choose a friendly message >> instead of saying "unknown tag" only. >> OBSOLETE is not used anywhere in this commit. Although HTML5 has some >> obsolete features, >> [JDK-8215577](https://bugs.openjdk.java.net/browse/JDK-8215577) didn't >> define them as valid features if my understanding is correct. So I chose >> not to allow obsolete features in order to avoid inconsistency. > > For both `ElemKind` and `AttrKind` there only seem to be two kinds: > * valid > * previously valid > > For these two cases, `OK` is obviously reasonable for `valid`, but `OBSOLETE` > seems a better fit than `UNSUPPORTED`, but you could also use `HTML4` or > `OLD_HTML4` or something like that to indicate why we're keeping the name > around for better messages. Or, stay with `UNSUPPORTED` but add a doc > comment explaining that it was previously supported but no longer supported Ok, I will use `HTML4`. - PR: https://git.openjdk.java.net/jdk/pull/893
Re: RFR: 8247957: remove doclint support for HTML 4 [v3]
On Thu, 17 Dec 2020 04:52:48 GMT, Jonathan Gibbons wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint.properties
>> line 36:
>>
>>> 34: dc.attr.img.border.not.number = attribute "border" for img is not a
>>> number
>>> 35: dc.attr.table.border.not.valid = attribute "border" for table only
>>> accepts "" or "1", use CSS instead: {0}
>>> 36: dc.attr.table.border.not.number = attribute "border" for table is not a
>>> number
>>
>> suggest dropping "use CSS instead"
>
> The wording of `attribute "border" for img is not a number` seems strange.
> Suggest something like `invalid value for attribute "border": {0}`
Ok, I will fix that.
-
PR: https://git.openjdk.java.net/jdk/pull/893
Re: RFR: 8247957: remove doclint support for HTML 4 [v3]
On Thu, 17 Dec 2020 01:40:14 GMT, Yoshiki Sato wrote:
>> HTML4 is no longer supported in javadoc.
>>
>> doclint needs to drop HTML4 support as well.
>> The changes consist of:
>> * Removing jdk.javadoc.internal.doclint.HtmlVersion and its references.
>> * Sorting out supported tags and attributes in HTML5 (including fix
>> incorrect permission of valign in TH, TR, TD, THEAD and TBODY)
>> * Modifying test code and expected outputs to be checked in HTML5
>
> Yoshiki Sato has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 8257204 and 8256313
> 8257204: Remove usage of -Xhtmlversion option from javac
> 8256313: JavaCompilation.gmk needs to be updated not to use
> --doclint-format html5 option
Mostly OK; some minor suggestions
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint.properties
line 36:
> 34: dc.attr.img.border.not.number = attribute "border" for img is not a number
> 35: dc.attr.table.border.not.valid = attribute "border" for table only
> accepts "" or "1", use CSS instead: {0}
> 36: dc.attr.table.border.not.number = attribute "border" for table is not a
> number
suggest dropping "use CSS instead"
-
PR: https://git.openjdk.java.net/jdk/pull/893
Re: RFR: 8247957: remove doclint support for HTML 4 [v3]
On Thu, 17 Dec 2020 04:50:59 GMT, Jonathan Gibbons wrote:
>> Yoshiki Sato has refreshed the contents of this pull request, and previous
>> commits have been removed. The incremental views will show differences
>> compared to the previous content of the PR. The pull request contains one
>> new commit since the last revision:
>>
>> 8257204 and 8256313
>> 8257204: Remove usage of -Xhtmlversion option from javac
>> 8256313: JavaCompilation.gmk needs to be updated not to use
>> --doclint-format html5 option
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint.properties
> line 36:
>
>> 34: dc.attr.img.border.not.number = attribute "border" for img is not a
>> number
>> 35: dc.attr.table.border.not.valid = attribute "border" for table only
>> accepts "" or "1", use CSS instead: {0}
>> 36: dc.attr.table.border.not.number = attribute "border" for table is not a
>> number
>
> suggest dropping "use CSS instead"
The wording of `attribute "border" for img is not a number` seems strange.
Suggest something like `invalid value for attribute "border": {0}`
-
PR: https://git.openjdk.java.net/jdk/pull/893
Re: RFR: 8247957: remove doclint support for HTML 4 [v3]
On Thu, 12 Nov 2020 03:10:01 GMT, Yoshiki Sato wrote: >> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/HtmlTag.java line >> 410: >> >>> 408: OBSOLETE, >>> 409: UNSUPPORTED >>> 410: } >> >> On one hand, I don't think we need this level of detail, but on the other, I >> see it closely matches `AttrKind`, so OK. >> >> Is there are useful distinction between INVALID / OBSOLETE / UNSUPPORTED ? > > OK: valid > OBSOLETE: obsolete, deprecated, but still supported (valid) > UNSUPPORTED: ever supported but no longer supported (invalid) > INVALID: the rest of others (invalid) > > UNSUPPORTED can be used if we would like to choose a friendly message instead > of saying "unknown tag" only. > OBSOLETE is not used anywhere in this commit. Although HTML5 has some > obsolete features, > [JDK-8215577](https://bugs.openjdk.java.net/browse/JDK-8215577) didn't define > them as valid features if my understanding is correct. So I chose not to > allow obsolete features in order to avoid inconsistency. For both `ElemKind` and `AttrKind` there only seem to be two kinds: * valid * previously valid For these two cases, `OK` is obviously reasonable for `valid`, but `OBSOLETE` seems a better fit than `UNSUPPORTED`, but you could also use `HTML4` or `OLD_HTML4` or something like that to indicate why we're keeping the name around for better messages. Or, stay with `UNSUPPORTED` but add a doc comment explaining that it was previously supported but no longer supported - PR: https://git.openjdk.java.net/jdk/pull/893
Re: RFR: 8247957: remove doclint support for HTML 4 [v3]
> HTML4 is no longer supported in javadoc. > > doclint needs to drop HTML4 support as well. > The changes consist of: > * Removing jdk.javadoc.internal.doclint.HtmlVersion and its references. > * Sorting out supported tags and attributes in HTML5 (including fix incorrect > permission of valign in TH, TR, TD, THEAD and TBODY) > * Modifying test code and expected outputs to be checked in HTML5 Yoshiki Sato has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8257204 and 8256313 8257204: Remove usage of -Xhtmlversion option from javac 8256313: JavaCompilation.gmk needs to be updated not to use --doclint-format html5 option - Changes: - all: https://git.openjdk.java.net/jdk/pull/893/files - new: https://git.openjdk.java.net/jdk/pull/893/files/294b3cce..30db3882 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=893=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=893=01-02 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/893.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/893/head:pull/893 PR: https://git.openjdk.java.net/jdk/pull/893
Re: [jdk16] RFR: JDK-8247994: Localize javadoc search [v2]
> This is for JDK16, as a precursor to fixing JDK-8258002. > > While it is good to be using localized strings in the generated output, the > significance for JDK-8258002 is that the strings are now obtained from a > resource file, and not hardcoded in JavaScript file itself. > > The source file `search.js` is renamed to `search.js.template`, and (unlike > other template files) is copied as-is into the generated image. The values in > the template are resolved when javadoc is executed, depending on the locale > in use at the time. Because of the change in the file's extension, two > makefiles are updated to accommodate the new extension: one is for the > "interim" javadoc used to generate the API docs; the other is for the primary > javadoc in the main JDK image. Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into localize.search.js - JDK-8247994: Localize javadoc search - Changes: - all: https://git.openjdk.java.net/jdk16/pull/16/files - new: https://git.openjdk.java.net/jdk16/pull/16/files/8eb7f27b..ec85 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk16=16=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk16=16=00-01 Stats: 1532 lines in 52 files changed: 1207 ins; 121 del; 204 mod Patch: https://git.openjdk.java.net/jdk16/pull/16.diff Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/16/head:pull/16 PR: https://git.openjdk.java.net/jdk16/pull/16
Re: [jdk16] RFR: JDK-8247994: Localize javadoc search
On Mon, 14 Dec 2020 09:11:31 GMT, Hannes Wallnöfer wrote: >> This is for JDK16, as a precursor to fixing JDK-8258002. >> >> While it is good to be using localized strings in the generated output, the >> significance for JDK-8258002 is that the strings are now obtained from a >> resource file, and not hardcoded in JavaScript file itself. >> >> The source file `search.js` is renamed to `search.js.template`, and (unlike >> other template files) is copied as-is into the generated image. The values >> in the template are resolved when javadoc is executed, depending on the >> locale in use at the time. Because of the change in the file's extension, >> two makefiles are updated to accommodate the new extension: one is for the >> "interim" javadoc used to generate the API docs; the other is for the >> primary javadoc in the main JDK image. > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/search.js.template > line 40: > >> 38: var MIN_RESULTS = 3; >> 39: var MAX_RESULTS = 500; >> 40: var UNNAMED = "##REPLACE:doclet.search.unnamed##"; > > `` is not a string that is ever shown to the user, it is what is > used by javadoc to denote the default package (see > `toolkit.util.DocletConstants.DEFAULT_PACKAGE_NAME`). This variable shouldn't > be localized as that would break behaviour for code in the default package > (and show the localized string as package name, instead of no package name). Noted, thanks - PR: https://git.openjdk.java.net/jdk16/pull/16
Integrated: 8258411: Move module set configuration from Modules.gmk to conf dir
On Tue, 15 Dec 2020 16:11:45 GMT, Magnus Ihse Bursie wrote: > The hard-coded list of modules in `make/common/Modules.gmk` has always been a > wart in the build system. We pride ourself on using discovery instead of > hard-coded list. In this case, it is not possible to do do auto-discovery, > since the different module sets are configured, not determined. > > Thus the actual lists of module sets should move to the `make/conf` directory. > > This is the first patch in a series where I will move configuration values > spread over the build system into the designated `make/conf` directory. This pull request has now been integrated. Changeset: a244b822 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/a244b822 Stats: 393 lines in 5 files changed: 223 ins; 157 del; 13 mod 8258411: Move module set configuration from Modules.gmk to conf dir Reviewed-by: alanb, mchung - PR: https://git.openjdk.java.net/jdk/pull/1781
Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v5]
On Wed, 16 Dec 2020 18:36:25 GMT, Magnus Ihse Bursie wrote: >> The hard-coded list of modules in `make/common/Modules.gmk` has always been >> a wart in the build system. We pride ourself on using discovery instead of >> hard-coded list. In this case, it is not possible to do do auto-discovery, >> since the different module sets are configured, not determined. >> >> Thus the actual lists of module sets should move to the `make/conf` >> directory. >> >> This is the first patch in a series where I will move configuration values >> spread over the build system into the designated `make/conf` directory. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Changes as requested by reviewers Thanks for the updates. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1781
Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v5]
On Wed, 16 Dec 2020 18:36:25 GMT, Magnus Ihse Bursie wrote: >> The hard-coded list of modules in `make/common/Modules.gmk` has always been >> a wart in the build system. We pride ourself on using discovery instead of >> hard-coded list. In this case, it is not possible to do do auto-discovery, >> since the different module sets are configured, not determined. >> >> Thus the actual lists of module sets should move to the `make/conf` >> directory. >> >> This is the first patch in a series where I will move configuration values >> spread over the build system into the designated `make/conf` directory. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Changes as requested by reviewers Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1781
Integrated: 8258447: Move make/hotspot/hotspot.script to make/scripts
On Tue, 15 Dec 2020 23:28:25 GMT, Magnus Ihse Bursie wrote: > The hotspot launcher script is misplaced among the hotspot make files. It > should move to make/scripts (and get a proper extension). This pull request has now been integrated. Changeset: ab5d581b Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/ab5d581b Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod 8258447: Move make/hotspot/hotspot.script to make/scripts Reviewed-by: dcubed - PR: https://git.openjdk.java.net/jdk/pull/1791
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Thu, 10 Dec 2020 23:07:52 GMT, Naoto Sato wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Move to share/data, and move jdwp.spec to java.se > > Reviewed changes to `characterdata`, `charsetmapping`, `cldr`, `currency`, > `lsrdata`, `tzdata`, and `unicodedata` with minor comment. Looks good overall. >I also think that the person most qualified to judge about charsetmapping is >@naotoj, which has already accepted this review as it is. Although I am the current RE for the charsets component, I succeeded it from Alan/Sherman, so I would like to hear Alan's opinion on this. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v5]
> The hard-coded list of modules in `make/common/Modules.gmk` has always been a > wart in the build system. We pride ourself on using discovery instead of > hard-coded list. In this case, it is not possible to do do auto-discovery, > since the different module sets are configured, not determined. > > Thus the actual lists of module sets should move to the `make/conf` directory. > > This is the first patch in a series where I will move configuration values > spread over the build system into the designated `make/conf` directory. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Changes as requested by reviewers - Changes: - all: https://git.openjdk.java.net/jdk/pull/1781/files - new: https://git.openjdk.java.net/jdk/pull/1781/files/199832a1..b7d393f0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1781=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1781=03-04 Stats: 14 lines in 3 files changed: 1 ins; 7 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/1781.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1781/head:pull/1781 PR: https://git.openjdk.java.net/jdk/pull/1781
Re: RFR: JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec) [v2]
On Thu, 5 Nov 2020 16:16:44 GMT, Pavel Rappo wrote: >> Jonathan Gibbons has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains 11 commits: >> >> - Fix merge issues; review feedback >> - Merge with master >> - allow rich content in createAnchorAndSearchIndex >> - update Docs.gmk to stop disabling spec tag >> - fix TestSpecTag.testEncodedURI >> - fix tests >> - remove support to workaround legacy @spec tag >> - Merge remote-tracking branch 'upstream/master' into new-spec-tag >> - fix trailing whitespace in test >> - temporarily allow existing legacy usage `@spec JPMS` `@spec jsr-51` >> - ... and 1 more: >> https://git.openjdk.java.net/jdk/compare/804bd725...ed5512d9 > > 1. Thanks for incorporating my previous offline feedback. > 2. Since Hannes and Erik seem to have looked at everything else, I looked > mostly at changes in `src/jdk.compiler/share/classes/com/sun/source/**`, > which are good! > 3. There should be a corresponding but separate change to "Documentation > Comment Specification for the Standard Doclet". > 4. Can we use this new `@since` tag to refer to the spec at > `com/sun/tools/javac/parser/DocCommentParser.java:1116`? > 5. Should we specify in `com.sun.source.doctree.SpecTree` that both `url` and > `label` parts are mandatory? > 6. `DCSpec extends DCEndPosTree`, sigh... Although that is not a public API, > this design suggests we could improve that abstraction sometime later. Closing pull request, until we better decide the contents of the spec page - PR: https://git.openjdk.java.net/jdk/pull/790
Withdrawn: JDK-6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
On Thu, 22 Oct 2020 02:40:44 GMT, Jonathan Gibbons wrote:
> This introduces support for a new `@spec` tag that can be used as either an
> inline tag or as a block tag. It is used to identify references to external
> specifications, in such a way that the references can be collected together
> on a new summary page, called "Other Specifications", available from either
> the static INDEX page or the interactive search box.
>
> As an inline tag, the format is `{@spec url label}`, which is roughly
> translated to `label` in the generated docs.
>
> As a block tag, the format is simply
>
> @spec url label
>
> which is handled in a manner analogous to
>
> @see label
>
> The tag is notable for being the first standard/supported tag that can be
> used as either an inline or block tag. (We have had support for bimodal
> non-standard/custom tags for a while, such as `{@jls}` and `{@jvms}`.) To
> support bimodal standard tags, some changes to `DocCommentParser` are
> incorporated here.
>
> This change is only the _support_ for the new tag; it does _not_ include any
> changes to API docs to _use_ the new tag.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.java.net/jdk/pull/790
Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v2]
On Wed, 16 Dec 2020 14:40:14 GMT, Alan Bateman wrote: > javadoc-modules.conf is probably okay although someone finding this in the > repo might initially think it's the configuration for the javadoc modules. > That plus it sets DOCS_MODULES, so maybe it should be apidocs-modules.conf. I'm okay with apidocs-modules.conf or docs-modules.conf. It's good to match the make target name which makes it easier to understand what this configuration is for. > module-loader-map.conf works as the configuration file that defines > BOOT_MODULES and PLATFORM_MODULES. I think AGGREGATOR_MODULES should be > dropped and "java.se" added to PLATFORM_MODULES. If I remember correctly, > this was separated out in JDK 9 and 10 because of the java.se.ee aggregator > module (that one was removed in Java 11 by JEP 320). module-loader-map.conf works for me too. We can drop AGGREGATOR_MODULES since java.se is the sole aggregator module in JDK now. - PR: https://git.openjdk.java.net/jdk/pull/1781
Re: RFR: 8258447: Move make/hotspot/hotspot.script to make/scripts
On Tue, 15 Dec 2020 23:28:25 GMT, Magnus Ihse Bursie wrote: > The hotspot launcher script is misplaced among the hotspot make files. It > should move to make/scripts (and get a proper extension). The rename and makefile changes look good. I've used the script on occasion, but not recently (not in the last 6 months). - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1791
Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v2]
On Wed, 16 Dec 2020 13:51:50 GMT, Magnus Ihse Bursie wrote: >> The update to JRE_MODULES in Images.gmk resolves my comment above. However, >> the naming for the configuration is still a bit odd, e.g. >> module-sets-classloaders.conf should be something like >> module-loader-map.conf as used to generate ModuleLoaderMap.java in the build. > > @AlanBateman I don't have a problem with renaming the conf files, I just did > not know what you wanted them to be called. :-) I renamed > `module-sets-classloaders.conf` to `module-loader-map.conf`. Based on this, I > rename the other two files `javadoc-modules.conf` and > `build-module-sets.conf`, respectively. I hope this is okay. Otherwise, > please just let me know what you think they should be called. Thanks for the update. javadoc-modules.conf is probably okay although someone finding this in the repo might initially think it's the configuration for the javadoc modules. That plus it sets DOCS_MODULES, so maybe it should be apidocs-modules.conf. module-loader-map.conf works as the configuration file that defines BOOT_MODULES and PLATFORM_MODULES. I think AGGREGATOR_MODULES should be dropped and "java.se" added to PLATFORM_MODULES. If I remember correctly, this was separated out in JDK 9 and 10 because of the java.se.ee aggregator module (that one was removed in Java 11 by JEP 320). We should probably look at UPGRADEABLE_MODULES while we are here. This is the modules that are overriddable by way of excluding from the hashes stored in java.base (CreateJmods.gmk). I think it's okay to leave it in module-loader-map.conf because these modules are mapped to the platform class loader. Could we just rename it to UPGRADEABLE_PLATFORM_MODULES so that its a bit clearer (in Modules.gmk) as to why they are append to PLATFORM_MODULES? - PR: https://git.openjdk.java.net/jdk/pull/1781
Re: RFR: 8258477: Pre-submit testing using GitHub Actions should merge changes from target branch
On Wed, Dec 16, 2020 at 3:20 PM Robin Westberg wrote: > Hi Thomas, > > > On 16 Dec 2020, at 14:54, Thomas Stüfe wrote: > > > > Hi Robin, > > > > does this mean tests won't run on branches which cannot be merged with > the > > assumed target branch? > > No, if there’s a problem with doing the merge the tests will simply > continue without doing it, and just use the commit in the pull request > as-is. > > Best regards, > Robin Yes, that makes sense. Thank you! ..Thomas > > > > > Thanks, Thomas > > > > > > On Wed, Dec 16, 2020 at 11:55 AM Robin Westberg < > rwestb...@openjdk.java.net> > > wrote: > > > >> Normally when running GitHub Actions on a pull request, what is checked > >> out is the merge of the pull request with the latest changes on the > target > >> branch. This ensure that what is tested is as close as possible to what > >> will actually be the result of integrating said pull request. > >> > >> In our use of GitHub Actions, we don't run directly on pull requests but > >> instead allow contributors to run them in their own personal forks. In > that > >> context, there is no notion of a target branch. However, we can infer > the > >> target project and branch by encoding this in the workflow file. This > >> allows us to perform the same merge manually, and achieve the same > >> behaviour. > >> > >> The change also adds an option to disable this automated merge when > >> launching the workflow manually, which could be useful when > investigating > >> unexpected test failures. > >> > >> Note that downstream projects picking up this change will have to adapt > >> the workflow file to keep using these actions for pre-submit testing. > This > >> has been a common request from downstream projects, but could also be > done > >> in a separate change (or not at all). > >> > >> - > >> > >> Commit messages: > >> - Allow opting out of automated merge on manual runs > >> - Initial implementation > >> > >> Changes: https://git.openjdk.java.net/jdk/pull/1801/files > >> Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1801=00 > >> Issue: https://bugs.openjdk.java.net/browse/JDK-8258477 > >> Stats: 117 lines in 1 file changed: 116 ins; 0 del; 1 mod > >> Patch: https://git.openjdk.java.net/jdk/pull/1801.diff > >> Fetch: git fetch https://git.openjdk.java.net/jdk > >> pull/1801/head:pull/1801 > >> > >> PR: https://git.openjdk.java.net/jdk/pull/1801 > >> > >
Re: RFR: 8258477: Pre-submit testing using GitHub Actions should merge changes from target branch
Hi Thomas, > On 16 Dec 2020, at 14:54, Thomas Stüfe wrote: > > Hi Robin, > > does this mean tests won't run on branches which cannot be merged with the > assumed target branch? No, if there’s a problem with doing the merge the tests will simply continue without doing it, and just use the commit in the pull request as-is. Best regards, Robin > > Thanks, Thomas > > > On Wed, Dec 16, 2020 at 11:55 AM Robin Westberg > wrote: > >> Normally when running GitHub Actions on a pull request, what is checked >> out is the merge of the pull request with the latest changes on the target >> branch. This ensure that what is tested is as close as possible to what >> will actually be the result of integrating said pull request. >> >> In our use of GitHub Actions, we don't run directly on pull requests but >> instead allow contributors to run them in their own personal forks. In that >> context, there is no notion of a target branch. However, we can infer the >> target project and branch by encoding this in the workflow file. This >> allows us to perform the same merge manually, and achieve the same >> behaviour. >> >> The change also adds an option to disable this automated merge when >> launching the workflow manually, which could be useful when investigating >> unexpected test failures. >> >> Note that downstream projects picking up this change will have to adapt >> the workflow file to keep using these actions for pre-submit testing. This >> has been a common request from downstream projects, but could also be done >> in a separate change (or not at all). >> >> - >> >> Commit messages: >> - Allow opting out of automated merge on manual runs >> - Initial implementation >> >> Changes: https://git.openjdk.java.net/jdk/pull/1801/files >> Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1801=00 >> Issue: https://bugs.openjdk.java.net/browse/JDK-8258477 >> Stats: 117 lines in 1 file changed: 116 ins; 0 del; 1 mod >> Patch: https://git.openjdk.java.net/jdk/pull/1801.diff >> Fetch: git fetch https://git.openjdk.java.net/jdk >> pull/1801/head:pull/1801 >> >> PR: https://git.openjdk.java.net/jdk/pull/1801 >>
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Wed, 16 Dec 2020 10:12:54 GMT, Alan Bateman wrote: >> I think this is almost ready to be pushed, but I'd like to have someone from >> the client team review the changes for java.desktop as well. @prrace Any >> change you can have a look at this? > > I think it will be annoying to have the charset mapping tables and other data > in the src tree, have you looked at other alternatives? @AlanBateman Let me re-iterate: the data files are *not* "make" files. It is just as annoying to have team-specific data files in the make tree! So to keep things as they are is not an option. The fact that they currently reside there is just a continuation of the old view of make as a general dumping ground. I've requested this change since before Project Jigsaw. In fact, I opposed Erik's original Jigsaw patch on this ground, among other things. As a compromise, we agreed that it was to be fixed *after* Jigsaw, since that project had already dragged out in time for so long and was blocking the release. (See https://bugs.openjdk.java.net/browse/JDK-8055193 for details.) So what do you propose for alternative? A new top-level data directory? I mean, sure, we could have like `data/java.base/share/charsetmapping` instead. I personally think that makes less sense. I also think that the person most qualified to judge about charsetmapping is @naotoj, which has already accepted this review as it is. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v2]
On Wed, 16 Dec 2020 10:21:08 GMT, Alan Bateman wrote: >> @mlchung The entire point of this exercise is to *not* have hard-coded lists >> of modules in make files... >> >> Having hard-coded lists have come back to bite us, time after time again. We >> try to auto-discover everything that is possible. For these sets of modules, >> however, auto-discovery is not possible since these lists *define* what we >> mean by e.g. platform modules or an interim image. > > The update to JRE_MODULES in Images.gmk resolves my comment above. However, > the naming for the configuration is still a bit odd, e.g. > module-sets-classloaders.conf should be something like module-loader-map.conf > as used to generate ModuleLoaderMap.java in the build. @AlanBateman I don't have a problem with renaming the conf files, I just did not know what you wanted them to be called. :-) I renamed `module-sets-classloaders.conf` to `module-loader-map.conf`. Based on this, I rename the other two files `javadoc-modules.conf` and `build-module-sets.conf`, respectively. I hope this is okay. Otherwise, please just let me know what you think they should be called. - PR: https://git.openjdk.java.net/jdk/pull/1781
Re: RFR: 8258477: Pre-submit testing using GitHub Actions should merge changes from target branch
Hi Robin, does this mean tests won't run on branches which cannot be merged with the assumed target branch? Thanks, Thomas On Wed, Dec 16, 2020 at 11:55 AM Robin Westberg wrote: > Normally when running GitHub Actions on a pull request, what is checked > out is the merge of the pull request with the latest changes on the target > branch. This ensure that what is tested is as close as possible to what > will actually be the result of integrating said pull request. > > In our use of GitHub Actions, we don't run directly on pull requests but > instead allow contributors to run them in their own personal forks. In that > context, there is no notion of a target branch. However, we can infer the > target project and branch by encoding this in the workflow file. This > allows us to perform the same merge manually, and achieve the same > behaviour. > > The change also adds an option to disable this automated merge when > launching the workflow manually, which could be useful when investigating > unexpected test failures. > > Note that downstream projects picking up this change will have to adapt > the workflow file to keep using these actions for pre-submit testing. This > has been a common request from downstream projects, but could also be done > in a separate change (or not at all). > > - > > Commit messages: > - Allow opting out of automated merge on manual runs > - Initial implementation > > Changes: https://git.openjdk.java.net/jdk/pull/1801/files > Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1801=00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8258477 > Stats: 117 lines in 1 file changed: 116 ins; 0 del; 1 mod > Patch: https://git.openjdk.java.net/jdk/pull/1801.diff > Fetch: git fetch https://git.openjdk.java.net/jdk > pull/1801/head:pull/1801 > > PR: https://git.openjdk.java.net/jdk/pull/1801 >
Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v4]
> The hard-coded list of modules in `make/common/Modules.gmk` has always been a > wart in the build system. We pride ourself on using discovery instead of > hard-coded list. In this case, it is not possible to do do auto-discovery, > since the different module sets are configured, not determined. > > Thus the actual lists of module sets should move to the `make/conf` directory. > > This is the first patch in a series where I will move configuration values > spread over the build system into the designated `make/conf` directory. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Rename configuration files - Changes: - all: https://git.openjdk.java.net/jdk/pull/1781/files - new: https://git.openjdk.java.net/jdk/pull/1781/files/ef9d53cb..199832a1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1781=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1781=02-03 Stats: 3 lines in 4 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1781.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1781/head:pull/1781 PR: https://git.openjdk.java.net/jdk/pull/1781
Integrated: 8258420: Move URL configuration from Docs.gmk to conf dir
On Tue, 15 Dec 2020 17:50:46 GMT, Magnus Ihse Bursie wrote: > In `Docs.gmk` there are some hard-coded links to online URL documentation and > bug reporting locations. These should not reside in the make file per se, but > instead move to the `make/conf` directory. This pull request has now been integrated. Changeset: 6eca2960 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/6eca2960 Stats: 39 lines in 2 files changed: 33 ins; 6 del; 0 mod 8258420: Move URL configuration from Docs.gmk to conf dir Reviewed-by: alanb - PR: https://git.openjdk.java.net/jdk/pull/1785
Re: RFR: 8258420: Move URL configuration from Docs.gmk to conf dir
On Tue, 15 Dec 2020 17:50:46 GMT, Magnus Ihse Bursie wrote: > In `Docs.gmk` there are some hard-coded links to online URL documentation and > bug reporting locations. These should not reside in the make file per se, but > instead move to the `make/conf` directory. Okay. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1785
RFR: 8258477: Pre-submit testing using GitHub Actions should merge changes from target branch
Normally when running GitHub Actions on a pull request, what is checked out is the merge of the pull request with the latest changes on the target branch. This ensure that what is tested is as close as possible to what will actually be the result of integrating said pull request. In our use of GitHub Actions, we don't run directly on pull requests but instead allow contributors to run them in their own personal forks. In that context, there is no notion of a target branch. However, we can infer the target project and branch by encoding this in the workflow file. This allows us to perform the same merge manually, and achieve the same behaviour. The change also adds an option to disable this automated merge when launching the workflow manually, which could be useful when investigating unexpected test failures. Note that downstream projects picking up this change will have to adapt the workflow file to keep using these actions for pre-submit testing. This has been a common request from downstream projects, but could also be done in a separate change (or not at all). - Commit messages: - Allow opting out of automated merge on manual runs - Initial implementation Changes: https://git.openjdk.java.net/jdk/pull/1801/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1801=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258477 Stats: 117 lines in 1 file changed: 116 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1801.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1801/head:pull/1801 PR: https://git.openjdk.java.net/jdk/pull/1801
Re: RFR: 8258411: Move module set configuration from Modules.gmk to conf dir [v2]
On Wed, 16 Dec 2020 00:14:02 GMT, Magnus Ihse Bursie wrote: >> Can any of `INTERIM_IMAGE_MODULES` , `HOTSPOT_MODULES` and >> `LANGTOOLS_MODULES` be inlined in the appropriate .gmk file? >> >> `INTERIM_IMAGE_MODULES` is for building interim image. If it has to be >> defined in a conf file, I like its name be explicit and match the target or >> makefile, for example, `interim-images.conf` or `InterimImages.conf`. >> This way I can tell what this conf file intends for. What do you think? > > @mlchung The entire point of this exercise is to *not* have hard-coded lists > of modules in make files... > > Having hard-coded lists have come back to bite us, time after time again. We > try to auto-discover everything that is possible. For these sets of modules, > however, auto-discovery is not possible since these lists *define* what we > mean by e.g. platform modules or an interim image. The update to JRE_MODULES in Images.gmk resolves my comment above. However, the naming for the configuration is still a bit odd, e.g. module-sets-classloaders.conf should be something like module-loader-map.conf as used to generate ModuleLoaderMap.java in the build. - PR: https://git.openjdk.java.net/jdk/pull/1781
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 15 Dec 2020 22:52:30 GMT, Magnus Ihse Bursie wrote: >> Reviewed changes to `characterdata`, `charsetmapping`, `cldr`, `currency`, >> `lsrdata`, `tzdata`, and `unicodedata` with minor comment. Looks good >> overall. > > I think this is almost ready to be pushed, but I'd like to have someone from > the client team review the changes for java.desktop as well. @prrace Any > change you can have a look at this? I think it will be annoying to have the charset mapping tables and other data in the src tree, have you looked at other alternatives? - PR: https://git.openjdk.java.net/jdk/pull/1611
