Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On Wed, 16 Mar 2022 21:56:53 GMT, Magnus Ihse Bursie wrote: >> make/modules/jdk.charsets/Gensrc.gmk line 32: >> >>> 30: # Generate files using the charsetmapping tool >>> 31: # >>> 32: CHARSET_DATA_DIR := $(TOPDIR)/src/java.base/share/data/charsetmapping >> >> Is it intentional to leave `java.base` literal here, or should it be >> replaced with `$(MODULE_SRC)`? I see this inconsistency in other tools' >> `gensrc.gmk` too > > This is part of the weirdness of charsetmapping that Alan talks about. The > charsetmapping data is shared between java.base and jdk.charsets in a way > that makes it non-trivial to disentangle. > > So this reference to java.base is quite intentional -- replacing it with > $(MODULE_SRC) would have pointed to src/jdk.charsets instead of > src/java.base, which would have been incorrect. Thanks for reminding me. Then yes, I'd agree with Alan. It'd be much simpler to exclude this from this PR. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On Wed, 16 Mar 2022 21:31:08 GMT, Naoto Sato wrote: >> Magnus Ihse Bursie has updated the pull request with a new target base due >> to a merge or a rebase. The pull request now contains 12 commits: >> >> - Merge branch 'master' into shuffle-data-reborn >> - Fix merge >> - Merge tag 'jdk-19+13' into shuffle-data-reborn >> >>Added tag jdk-19+13 for changeset 5df2a057 >> - Move characterdata templates to share/classes/java/lang. >> - Update comment refering to "make" dir >> - Move new symbols to jdk.compiler >> - Merge branch 'master' into shuffle-data >> - Move macosxicons from share to macosx >> - Move to share/data, and move jdwp.spec to java.se >> - Update references in test >> - ... and 2 more: >> https://git.openjdk.java.net/jdk/compare/83d77186...598f740f > > make/modules/jdk.charsets/Gensrc.gmk line 32: > >> 30: # Generate files using the charsetmapping tool >> 31: # >> 32: CHARSET_DATA_DIR := $(TOPDIR)/src/java.base/share/data/charsetmapping > > Is it intentional to leave `java.base` literal here, or should it be replaced > with `$(MODULE_SRC)`? I see this inconsistency in other tools' `gensrc.gmk` > too This is part of the weirdness of charsetmapping that Alan talks about. The charsetmapping data is shared between java.base and jdk.charsets in a way that makes it non-trivial to disentangle. So this reference to java.base is quite intentional -- replacing it with $(MODULE_SRC) would have pointed to src/jdk.charsets instead of src/java.base, which would have been incorrect. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On Tue, 15 Mar 2022 23:50:20 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains 12 commits: > > - Merge branch 'master' into shuffle-data-reborn > - Fix merge > - Merge tag 'jdk-19+13' into shuffle-data-reborn > >Added tag jdk-19+13 for changeset 5df2a057 > - Move characterdata templates to share/classes/java/lang. > - Update comment refering to "make" dir > - Move new symbols to jdk.compiler > - Merge branch 'master' into shuffle-data > - Move macosxicons from share to macosx > - Move to share/data, and move jdwp.spec to java.se > - Update references in test > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/83d77186...598f740f Looks good, with minor comments. Also I am assuming you will modify the copyright year for these files before the merge. make/modules/jdk.charsets/Gensrc.gmk line 32: > 30: # Generate files using the charsetmapping tool > 31: # > 32: CHARSET_DATA_DIR := $(TOPDIR)/src/java.base/share/data/charsetmapping Is it intentional to leave `java.base` literal here, or should it be replaced with `$(MODULE_SRC)`? I see this inconsistency in other tools' `gensrc.gmk` too - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On 16/03/2022 08:44, Magnus Ihse Bursie wrote: : If you have such strong opinions on the data files shared between java.base and jdk.charsets/jdk.localedata, I propose we leave them in make/data for the time being, clean up the associated mess, and then work out where they actually should be. Does that sound okay to you? The concern, as before, is that it puts data files into src/java.base that are used by the build to generate classes/resources for the service provider modules. We also have the complication that the charsets to include in java.base varies by platform so the module/package for each charset is decided at build time. It's always been low-priority to re-visit that and not clear if we could even get to an agreement easily because there are IBM platforms that want EBCDIC and other double byte charsets whereas other platforms don't want these in java.base. So yes, if you can drop the move of the charset data and CLDR data from the patch then it will make it easier to discuss. You asked about the JDWP spec. This is the protocol spec that is used to generate the spec in HTML, and source files for the debugger front-end and backend (jdk.jdi and jdk.jdwp.agent modules). The "specs" directory might be right. There is another source file (jdwp-spec.md) that isn't in the src tree right now and they probably go together. -Alan
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On Tue, 15 Mar 2022 23:50:20 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains 12 commits: > > - Merge branch 'master' into shuffle-data-reborn > - Fix merge > - Merge tag 'jdk-19+13' into shuffle-data-reborn > >Added tag jdk-19+13 for changeset 5df2a057 > - Move characterdata templates to share/classes/java/lang. > - Update comment refering to "make" dir > - Move new symbols to jdk.compiler > - Merge branch 'master' into shuffle-data > - Move macosxicons from share to macosx > - Move to share/data, and move jdwp.spec to java.se > - Update references in test > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/83d77186...598f740f It might be helpful to go to the webrev instead: https://openjdk.github.io/cr/?repo=jdk=1611=05 Only the first dozen or so of files has actual changes; the rest are moved files. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On Tue, 15 Mar 2022 23:50:20 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains 12 commits: > > - Merge branch 'master' into shuffle-data-reborn > - Fix merge > - Merge tag 'jdk-19+13' into shuffle-data-reborn > >Added tag jdk-19+13 for changeset 5df2a057 > - Move characterdata templates to share/classes/java/lang. > - Update comment refering to "make" dir > - Move new symbols to jdk.compiler > - Merge branch 'master' into shuffle-data > - Move macosxicons from share to macosx > - Move to share/data, and move jdwp.spec to java.se > - Update references in test > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/83d77186...598f740f How are folks reviewing this huge PR ? My browser paints a few files and that's it, and the drop down list to "jump to" is too big to display - it says ! - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On 2022-03-16 07:31, Alan Bateman wrote:
Magnus,
This proposal does not address the previous concerns. As before, you
are proposing to put the data files used to generate the classes for
jdk.localedata and jdk.charsets into src/java.base and I don't think
we should do that. I really think this PR needs to be withdraw n or
closed until there is at least some agreement on placement. I know you
want to get the files moved out of the make tree but there are many
issues to work through before that can happen.
I'm sorry that you feel I did not properly address your concerns. You wrote:
"If you are moving them into the src tree then src/java.base (as you
have it) is best but will still have the ugly wart that some of these
mapping tables will be used to generate code for the jdk.charsets module."
which I interpreted as a need to file a follow-up issue to sort that
out, not a veto on the entire PR.
If you have such strong opinions on the data files shared between
java.base and jdk.charsets/jdk.localedata, I propose we leave them in
make/data for the time being, clean up the associated mess, and then
work out where they actually should be. Does that sound okay to you?
/Magnus
-Alan
On 15/03/2022 23:59, Magnus Ihse Bursie wrote:
On Tue, 15 Mar 2022 23:50:20 GMT, Magnus Ihse Bursie
wrote:
A lot (but not all) of the data in make/data is tied to a specific
module. For instance, the publicsuffixlist is used by java.base,
and fontconfig by java.desktop. (A few directories, like
mainmanifest, is *actually* used by make for the whole build.)
These data files should move to the module they belong to. The are,
after all, "source code" for that module that is "compiler" into
resulting deliverables, for that module. (But the "source code"
language is not Java or C, but typically a highly domain specific
language or data format, and the "compilation" is, often, a
specialized transformation.)
This misplacement of the data directory is most visible at code
review time. When such data is changed, most of the time build-dev
(or the new build label) is involved, even though this has nothing
to do with the build. While this is annoying, a worse problem is if
the actual team that needs to review the patch (i.e., the team
owning the module) is missed in the review.
### Modules reviewed
- [x] java.base
- [x] java.desktop
- [x] jdk.compiler
- [x] java.se
Magnus Ihse Bursie has updated the pull request with a new target
base due to a merge or a rebase. The pull request now contains 12
commits:
- Merge branch 'master' into shuffle-data-reborn
- Fix merge
- Merge tag 'jdk-19+13' into shuffle-data-reborn
Added tag jdk-19+13 for changeset 5df2a057
- Move characterdata templates to share/classes/java/lang.
- Update comment refering to "make" dir
- Move new symbols to jdk.compiler
- Merge branch 'master' into shuffle-data
- Move macosxicons from share to macosx
- Move to share/data, and move jdwp.spec to java.se
- Update references in test
- ... and 2 more:
https://git.openjdk.java.net/jdk/compare/83d77186...598f740f
I have carefully reviewed all PR comments, and the changes I made in
response to them. I believe I have resolved all requests from
reviewers. What remained to do was to create an informational JEP
about the new source structure, and file some follow-up issues.
I have now created and submitted a new informational JEP ("JDK Source
Structure"), available at
https://bugs.openjdk.java.net/browse/JDK-8283227. When creating this
JEP, it felt increasingly silly to just copy and extend the part
about src/$MODULE from JEP 201, so I extended it to cover a relevant
overview of the entire JDK source base structure. I actually think
this JEP has a good merit on its own, notwithstanding it being a
reviewer requirement for this PR.
I have also filed follow up issues for the non-standard
jdk.hotspot.agent `doc` and `test` directories
(https://bugs.openjdk.java.net/browse/JDK-8283197 and
https://bugs.openjdk.java.net/browse/JDK-8283198, respectively).
I have filed a follow up issue for continued efforts to clean up
charsetmapping, https://bugs.openjdk.java.net/browse/JDK-8283228.
There were two open questions:
* should jdwp.spec belong to specs directory instead of data
* should bin/idea.sh be changed to exclude data
but they sounded so exploratory that I decided not to open JBS issues
for them.
@wangweij @naotoj @prrace @erikj79 @jonathan-gibbons You have all
approved this PR at an older revision. Can you please reconfirm that
your approval stands for the latest revision? (Sorry for the mass ping)
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
Magnus,
This proposal does not address the previous concerns. As before, you are
proposing to put the data files used to generate the classes for
jdk.localedata and jdk.charsets into src/java.base and I don't think we
should do that. I really think this PR needs to be withdraw n or closed
until there is at least some agreement on placement. I know you want to
get the files moved out of the make tree but there are many issues to
work through before that can happen.
-Alan
On 15/03/2022 23:59, Magnus Ihse Bursie wrote:
On Tue, 15 Mar 2022 23:50:20 GMT, Magnus Ihse Bursie wrote:
A lot (but not all) of the data in make/data is tied to a specific module. For
instance, the publicsuffixlist is used by java.base, and fontconfig by
java.desktop. (A few directories, like mainmanifest, is *actually* used by make
for the whole build.)
These data files should move to the module they belong to. The are, after all, "source code" for that module
that is "compiler" into resulting deliverables, for that module. (But the "source code" language is
not Java or C, but typically a highly domain specific language or data format, and the "compilation" is,
often, a specialized transformation.)
This misplacement of the data directory is most visible at code review time.
When such data is changed, most of the time build-dev (or the new build label)
is involved, even though this has nothing to do with the build. While this is
annoying, a worse problem is if the actual team that needs to review the patch
(i.e., the team owning the module) is missed in the review.
### Modules reviewed
- [x] java.base
- [x] java.desktop
- [x] jdk.compiler
- [x] java.se
Magnus Ihse Bursie has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 12 commits:
- Merge branch 'master' into shuffle-data-reborn
- Fix merge
- Merge tag 'jdk-19+13' into shuffle-data-reborn
Added tag jdk-19+13 for changeset 5df2a057
- Move characterdata templates to share/classes/java/lang.
- Update comment refering to "make" dir
- Move new symbols to jdk.compiler
- Merge branch 'master' into shuffle-data
- Move macosxicons from share to macosx
- Move to share/data, and move jdwp.spec to java.se
- Update references in test
- ... and 2 more: https://git.openjdk.java.net/jdk/compare/83d77186...598f740f
I have carefully reviewed all PR comments, and the changes I made in response
to them. I believe I have resolved all requests from reviewers. What remained
to do was to create an informational JEP about the new source structure, and
file some follow-up issues.
I have now created and submitted a new informational JEP ("JDK Source
Structure"), available at https://bugs.openjdk.java.net/browse/JDK-8283227. When
creating this JEP, it felt increasingly silly to just copy and extend the part about
src/$MODULE from JEP 201, so I extended it to cover a relevant overview of the entire JDK
source base structure. I actually think this JEP has a good merit on its own,
notwithstanding it being a reviewer requirement for this PR.
I have also filed follow up issues for the non-standard jdk.hotspot.agent `doc`
and `test` directories (https://bugs.openjdk.java.net/browse/JDK-8283197 and
https://bugs.openjdk.java.net/browse/JDK-8283198, respectively).
I have filed a follow up issue for continued efforts to clean up
charsetmapping, https://bugs.openjdk.java.net/browse/JDK-8283228.
There were two open questions:
* should jdwp.spec belong to specs directory instead of data
* should bin/idea.sh be changed to exclude data
but they sounded so exploratory that I decided not to open JBS issues for them.
@wangweij @naotoj @prrace @erikj79 @jonathan-gibbons You have all approved
this PR at an older revision. Can you please reconfirm that your approval
stands for the latest revision? (Sorry for the mass ping)
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On Tue, 15 Mar 2022 23:50:20 GMT, Magnus Ihse Bursie wrote:
>> A lot (but not all) of the data in make/data is tied to a specific module.
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by
>> make for the whole build.)
>>
>> These data files should move to the module they belong to. The are, after
>> all, "source code" for that module that is "compiler" into resulting
>> deliverables, for that module. (But the "source code" language is not Java
>> or C, but typically a highly domain specific language or data format, and
>> the "compilation" is, often, a specialized transformation.)
>>
>> This misplacement of the data directory is most visible at code review time.
>> When such data is changed, most of the time build-dev (or the new build
>> label) is involved, even though this has nothing to do with the build. While
>> this is annoying, a worse problem is if the actual team that needs to review
>> the patch (i.e., the team owning the module) is missed in the review.
>>
>> ### Modules reviewed
>>
>> - [x] java.base
>> - [x] java.desktop
>> - [x] jdk.compiler
>> - [x] java.se
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to
> a merge or a rebase. The pull request now contains 12 commits:
>
> - Merge branch 'master' into shuffle-data-reborn
> - Fix merge
> - Merge tag 'jdk-19+13' into shuffle-data-reborn
>
>Added tag jdk-19+13 for changeset 5df2a057
> - Move characterdata templates to share/classes/java/lang.
> - Update comment refering to "make" dir
> - Move new symbols to jdk.compiler
> - Merge branch 'master' into shuffle-data
> - Move macosxicons from share to macosx
> - Move to share/data, and move jdwp.spec to java.se
> - Update references in test
> - ... and 2 more:
> https://git.openjdk.java.net/jdk/compare/83d77186...598f740f
I have carefully reviewed all PR comments, and the changes I made in response
to them. I believe I have resolved all requests from reviewers. What remained
to do was to create an informational JEP about the new source structure, and
file some follow-up issues.
I have now created and submitted a new informational JEP ("JDK Source
Structure"), available at https://bugs.openjdk.java.net/browse/JDK-8283227.
When creating this JEP, it felt increasingly silly to just copy and extend the
part about src/$MODULE from JEP 201, so I extended it to cover a relevant
overview of the entire JDK source base structure. I actually think this JEP has
a good merit on its own, notwithstanding it being a reviewer requirement for
this PR.
I have also filed follow up issues for the non-standard jdk.hotspot.agent `doc`
and `test` directories (https://bugs.openjdk.java.net/browse/JDK-8283197 and
https://bugs.openjdk.java.net/browse/JDK-8283198, respectively).
I have filed a follow up issue for continued efforts to clean up
charsetmapping, https://bugs.openjdk.java.net/browse/JDK-8283228.
There were two open questions:
* should jdwp.spec belong to specs directory instead of data
* should bin/idea.sh be changed to exclude data
but they sounded so exploratory that I decided not to open JBS issues for them.
@wangweij @naotoj @prrace @erikj79 @jonathan-gibbons You have all approved
this PR at an older revision. Can you please reconfirm that your approval
stands for the latest revision? (Sorry for the mass ping)
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
> A lot (but not all) of the data in make/data is tied to a specific module. > For instance, the publicsuffixlist is used by java.base, and fontconfig by > java.desktop. (A few directories, like mainmanifest, is *actually* used by > make for the whole build.) > > These data files should move to the module they belong to. The are, after > all, "source code" for that module that is "compiler" into resulting > deliverables, for that module. (But the "source code" language is not Java or > C, but typically a highly domain specific language or data format, and the > "compilation" is, often, a specialized transformation.) > > This misplacement of the data directory is most visible at code review time. > When such data is changed, most of the time build-dev (or the new build > label) is involved, even though this has nothing to do with the build. While > this is annoying, a worse problem is if the actual team that needs to review > the patch (i.e., the team owning the module) is missed in the review. > > ### Modules reviewed > > - [x] java.base > - [x] java.desktop > - [x] jdk.compiler > - [x] java.se Magnus Ihse Bursie has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - Merge branch 'master' into shuffle-data-reborn - Fix merge - Merge tag 'jdk-19+13' into shuffle-data-reborn Added tag jdk-19+13 for changeset 5df2a057 - Move characterdata templates to share/classes/java/lang. - Update comment refering to "make" dir - Move new symbols to jdk.compiler - Merge branch 'master' into shuffle-data - Move macosxicons from share to macosx - Move to share/data, and move jdwp.spec to java.se - Update references in test - ... and 2 more: https://git.openjdk.java.net/jdk/compare/83d77186...598f740f - Changes: https://git.openjdk.java.net/jdk/pull/1611/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1611=05 Stats: 85 lines in 1695 files changed: 4 ins; 1 del; 80 mod Patch: https://git.openjdk.java.net/jdk/pull/1611.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1611/head:pull/1611 PR: https://git.openjdk.java.net/jdk/pull/1611
