Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Wed, 16 Dec 2020 18:34:37 GMT, Naoto Sato wrote: >>> @AlanBateman The process of modularization was not fully completed with >>> Project Jigsaw, and a few ugly warts remained. I was under the impression >>> that these should be addressed in follow-up fixes, but this was >>> unfortunately never done. Charsets and cldrconverter were both split >>> between a core portion in java.base and the rest in jdk.charsets and >>> jdk.localedata, respectively, but the split was never handled properly, but >>> just "duct taped" in place. >>> >>> I chose to put the data files used for both java.base and the "additional" >>> modules in java.base, based on the comment that Naoto made in >>> https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027044.html: >>> >>> > As to charsetmapping and cldrconverter, I believe they can reside in >>> > java.base, as jdk.charsets and jdk.localedata modules depend on it. >>> >>> Of course it would be preferable to make a proper split, but that requires >>> work done by the component teams to break the modules apart. >>> >>> Specifically for make/modules/jdk.charsets/Gensrc.gmk; the code in that >>> file is more or less duplicated in >>> make/modules/java.base/gensrc/GensrcCharsetMapping.gmk, since the same data >>> set is processed twice, once for java.base and once for jdk.charsets. I >>> don't think that means that make/modules/jdk.charsets/Gensrc.gmk should >>> move to any other place. >> >> I still stand by what I wrote above. It's best to put data in java.base for >> charsets/localedata. Otherwise we would have to duplicate some in each >> modules source directory. > >>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. @naotoj @AlanBateman I have now rolled back any changes to make/data/cldr and make/data/charsetmapping. I have also updated copyright years; thanks for reminding me Naoto! - PR: https://git.openjdk.java.net/jdk/pull/1611
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: 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: 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
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 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? > test/jdk/java/util/Locale/LSRDataTest.java line 57: > >> 55: // path to the lsr file from the make folder, this test relies on the >> 56: // relative path to the file in the make folder, considering >> 57: // test and make will always exist in the same jdk layout > > The comment refers to the "make" folder. (line 55 as well). Fixed. Thank you for noticing! - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Mon, 7 Dec 2020 14:27:45 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 >> >> - [ ] java.base >> - [ ] java.desktop >> - [x] jdk.compiler >> - [ ] java.se > > 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. test/jdk/java/util/Locale/LSRDataTest.java line 57: > 55: // path to the lsr file from the make folder, this test relies on the > 56: // relative path to the file in the make folder, considering > 57: // test and make will always exist in the same jdk layout The comment refers to the "make" folder. (line 55 as well). - 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 [v2]
On Mon, 7 Dec 2020 14:27:45 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 >> >> - [ ] java.base >> - [ ] java.desktop >> - [x] jdk.compiler >> - [ ] java.se > > 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 The security-related part (cacerts, blacklisted.certs, publicsuffxlist) looks fine to me. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 17:33:16 GMT, Alan Bateman wrote: >> The mapping and nr tables, and the *-X.java.template files in >> make/data/charsetmapping are used to generate source code for the java.base >> and jdk.charsets modules. The stdcs-$OS files configure the package and >> module that each charset go into. If the tables used to generate the source >> files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk >> will probably need a new home too. > >> @AlanBateman The process of modularization was not fully completed with >> Project Jigsaw, and a few ugly warts remained. I was under the impression >> that these should be addressed in follow-up fixes, but this was >> unfortunately never done. Charsets and cldrconverter were both split between >> a core portion in java.base and the rest in jdk.charsets and jdk.localedata, >> respectively, but the split was never handled properly, but just "duct >> taped" in place. > > This is a complicated area of the build, not really a Project Jigsaw issue. > It's complicated because the source code for the charsets is generated at > build time and the set of non-standard charsets included in java.base varies > by platform, e.g. there's are several IBMxxx charsets in java.base when > building on AIX that are not interesting to include in java.base on other > platforms. This means we can't split up the mapping tables in > make/data/charsetmapping and put them in different directories. 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. @AlanBateman @mlchung @naotoj I can certainly anticipate follow-up cleanups on this patch. In fact, I think this patch has the potential to be a catalyst for such change, since the data that has been "hidden away" in `make` now becomes more visible to the teams that are capable of doing something about it. (With that said, of course the build team will assist in helping to clear up messy code structure, as we always do.) But I think we should be restrictive in trying too hard to make everything right in this single patch; it's better to get things approximately right and then go through the "warts" one by one and solve them properly. @AlanBateman What I meant by Jigsaw was that when the monolithic source code were modularized, the separation of concern between java.base and jdk.charsets/jdk.localedata was not complete, compared to (more or less) all other modules. It might very well be the case that we will never be able to make such a separation; but, prior to Jigsaw, this was not a "wart". It only became a code hygiene issue when some of the charsets and localedata was extraced from java.base. @mlchung My gut reaction is that we should not change idea.sh. First of all, kind of the purpose of this move is to make it clear to module developers the full set of materials their module consists of. That purpose would be sort of defeated, if we were to hide this in a popular IDE configuration. Secondly, I doubt this will add any performance difference. Listing additional files in the workspace is unlikely to do much, unless you actively open and/or interact with these files. But if you are worried, please fetch the PR (Skara adds instructions in the body) and try it out yourself! - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 17:33:16 GMT, Alan Bateman wrote: >> The mapping and nr tables, and the *-X.java.template files in >> make/data/charsetmapping are used to generate source code for the java.base >> and jdk.charsets modules. The stdcs-$OS files configure the package and >> module that each charset go into. If the tables used to generate the source >> files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk >> will probably need a new home too. > >> @AlanBateman The process of modularization was not fully completed with >> Project Jigsaw, and a few ugly warts remained. I was under the impression >> that these should be addressed in follow-up fixes, but this was >> unfortunately never done. Charsets and cldrconverter were both split between >> a core portion in java.base and the rest in jdk.charsets and jdk.localedata, >> respectively, but the split was never handled properly, but just "duct >> taped" in place. > > This is a complicated area of the build, not really a Project Jigsaw issue. > It's complicated because the source code for the charsets is generated at > build time and the set of non-standard charsets included in java.base varies > by platform, e.g. there's are several IBMxxx charsets in java.base when > building on AIX that are not interesting to include in java.base on other > platforms. This means we can't split up the mapping tables in > make/data/charsetmapping and put them in different directories. 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. > @AlanBateman The process of modularization was not fully completed with > Project Jigsaw, and a few ugly warts remained. I was under the impression > that these should be addressed in follow-up fixes, but this was unfortunately > never done. Charsets and cldrconverter were both split between a core portion > in java.base and the rest in jdk.charsets and jdk.localedata, respectively, > but the split was never handled properly, but just "duct taped" in place. > > I chose to put the data files used for both java.base and the "additional" > modules in java.base, based on the comment that Naoto made in > https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027044.html: > > > As to charsetmapping and cldrconverter, I believe they can reside in > > java.base, as jdk.charsets and jdk.localedata modules depend on it. > > Of course it would be preferable to make a proper split, but that requires > work done by the component teams to break the modules apart. > > Specifically for make/modules/jdk.charsets/Gensrc.gmk; the code in that file > is more or less duplicated in > make/modules/java.base/gensrc/GensrcCharsetMapping.gmk, since the same data > set is processed twice, once for java.base and once for jdk.charsets. I don't > think that means that make/modules/jdk.charsets/Gensrc.gmk should move to any > other place. I still stand by what I wrote above. It's best to put data in java.base for charsets/localedata. Otherwise we would have to duplicate some in each modules source directory. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 18:16:15 GMT, Mandy Chung wrote: >> @wangweij Moving build tools is a related, but separate, question, which is >> addressed by https://bugs.openjdk.java.net/browse/JDK-8241463. >> >> I send out a review earlier this year (see >> https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027038.html), >> but there were differing opinions on what the proper placement of these >> tools should be, and the discussion kind of fizzled out without reaching a >> conclusion. > > @magicus @erikj79 it's now clearly stated that you want everything under > `make` owned by the build team, which is fair. You are right that `make` has > been easily considered as a dumping ground and it's time to define a clean > structure. > > I reviewed this patch and this looks okay to me. Some follow-up questions > and follow-on cleanup for example "should jdwp.spec belong to `specs` > directory vs `data`? There are platform-specific data (such as charsets) > which has been special-case in the makefile and they need follow-on clean up. > I agree that this should be cleaned up by the component teams and not part > of this PR. With these new files showing up in under `src` directory, should `bin/idea.sh` be changed to exclude `data` to avoid incurring costs in loading JDK project from IDE that many of us use for development? - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 16:19:05 GMT, Magnus Ihse Bursie wrote: >> Is there a plan to move make/jdk/src/classes/build/tools/intpoly into >> java.base as well? >> >> Update: I see all subdirs in tools are still there. > > @wangweij Moving build tools is a related, but separate, question, which is > addressed by https://bugs.openjdk.java.net/browse/JDK-8241463. > > I send out a review earlier this year (see > https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027038.html), > but there were differing opinions on what the proper placement of these tools > should be, and the discussion kind of fizzled out without reaching a > conclusion. @magicus @erikj79 it's now clearly stated that you want everything under `make` owned by the build team, which is fair. You are right that `make` has been easily considered as a dumping ground and it's time to define a clean structure. I reviewed this patch and this looks okay to me. Some follow-up questions and follow-on cleanup for example "should jdwp.spec belong to `specs` directory vs `data`? There are platform-specific data (such as charsets) which has been special-case in the makefile and they need follow-on clean up. I agree that this should be cleaned up by the component teams and not part of this PR. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 12:15:38 GMT, Alan Bateman wrote: >> Also, to clarify, for me there is a fundamental difference between >> `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files >> that are part of the module, owned by the content team, and the `$MODULE` >> part is essential to delineate this content. The latter is owned by the >> build team, and is just a convenient way to organize the build system within >> the `make` directory. So it's clearly a no-no to put anything but `.gmk` >> files in `make/modules/$MODULE`. > > The mapping and nr tables, and the *-X.java.template files in > make/data/charsetmapping are used to generate source code for the java.base > and jdk.charsets modules. The stdcs-$OS files configure the package and > module that each charset go into. If the tables used to generate the source > files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk > will probably need a new home too. > @AlanBateman The process of modularization was not fully completed with > Project Jigsaw, and a few ugly warts remained. I was under the impression > that these should be addressed in follow-up fixes, but this was unfortunately > never done. Charsets and cldrconverter were both split between a core portion > in java.base and the rest in jdk.charsets and jdk.localedata, respectively, > but the split was never handled properly, but just "duct taped" in place. This is a complicated area of the build, not really a Project Jigsaw issue. It's complicated because the source code for the charsets is generated at build time and the set of non-standard charsets included in java.base varies by platform, e.g. there's are several IBMxxx charsets in java.base when building on AIX that are not interesting to include in java.base on other platforms. This means we can't split up the mapping tables in make/data/charsetmapping and put them in different directories. 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. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 15:25:47 GMT, Weijun Wang wrote: >> The mapping and nr tables, and the *-X.java.template files in >> make/data/charsetmapping are used to generate source code for the java.base >> and jdk.charsets modules. The stdcs-$OS files configure the package and >> module that each charset go into. If the tables used to generate the source >> files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk >> will probably need a new home too. > > Is there a plan to move make/jdk/src/classes/build/tools/intpoly into > java.base as well? > > Update: I see all subdirs in tools are still there. @wangweij Moving build tools is a related, but separate, question, which is addressed by https://bugs.openjdk.java.net/browse/JDK-8241463. I send out a review earlier this year (see https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027038.html), but there were differing opinions on what the proper placement of these tools should be, and the discussion kind of fizzled out without reaching a conclusion. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 12:15:38 GMT, Alan Bateman wrote: >> Also, to clarify, for me there is a fundamental difference between >> `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files >> that are part of the module, owned by the content team, and the `$MODULE` >> part is essential to delineate this content. The latter is owned by the >> build team, and is just a convenient way to organize the build system within >> the `make` directory. So it's clearly a no-no to put anything but `.gmk` >> files in `make/modules/$MODULE`. > > The mapping and nr tables, and the *-X.java.template files in > make/data/charsetmapping are used to generate source code for the java.base > and jdk.charsets modules. The stdcs-$OS files configure the package and > module that each charset go into. If the tables used to generate the source > files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk > will probably need a new home too. @AlanBateman The process of modularization was not fully completed with Project Jigsaw, and a few ugly warts remained. I was under the impression that these should be addressed in follow-up fixes, but this was unfortunately never done. Charsets and cldrconverter were both split between a core portion in java.base and the rest in jdk.charsets and jdk.localedata, respectively, but the split was never handled properly, but just "duct taped" in place. I chose to put the data files used for both java.base and the "additional" modules in java.base, based on the comment that Naoto made in https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027044.html: > As to charsetmapping and cldrconverter, I believe they can reside in java.base, as jdk.charsets and jdk.localedata modules depend on it. Of course it would be preferable to make a proper split, but that requires work done by the component teams to break the modules apart. Specifically for make/modules/jdk.charsets/Gensrc.gmk; the code in that file is more or less duplicated in make/modules/java.base/gensrc/GensrcCharsetMapping.gmk, since the same data set is processed twice, once for java.base and once for jdk.charsets. I don't think that means that make/modules/jdk.charsets/Gensrc.gmk should move to any other place. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 12:15:38 GMT, Alan Bateman wrote: >> Also, to clarify, for me there is a fundamental difference between >> `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files >> that are part of the module, owned by the content team, and the `$MODULE` >> part is essential to delineate this content. The latter is owned by the >> build team, and is just a convenient way to organize the build system within >> the `make` directory. So it's clearly a no-no to put anything but `.gmk` >> files in `make/modules/$MODULE`. > > The mapping and nr tables, and the *-X.java.template files in > make/data/charsetmapping are used to generate source code for the java.base > and jdk.charsets modules. The stdcs-$OS files configure the package and > module that each charset go into. If the tables used to generate the source > files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk > will probably need a new home too. Is there a plan to move make/jdk/src/classes/build/tools/intpoly into java.base as well? - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On 2020-12-08 00:30, Magnus Ihse Bursie wrote:
On Tue, 8 Dec 2020 02:40:43 GMT, Mandy Chung wrote:
I have reviewed all lines in the patch file with or near instances of
`jdk.compiler`
Hi Magnus,
I see the motivation of moving these build files for better identification of
ownership. Placing them under
`src/$MODULE/{share,$OS}/data` is one option. Given that skara will
automatically determine appropriate mailing lists of a PR, it seems that
`make/modules/$MODULE/data` can be another alternative that skara can include
this pattern in the mailing list configuration?? I don't yet have a strong
preference while I don't consider everything under `make` must be owned by the
build team though. Do you see one option is better than the other?
@mlchung If you don't have any strong preference, I implore you to accept this
change. I **vastly** prefer to move the data out of `make`!
This is not just about Skara tooling. When working with make files, autoconf and shell scripts,
there is no fancy IDE support, so you are stuck with simple text editors and tools like `grep`.
I've lost count on how many times I've had my grep searches blow up, since I happened to find e.g.
a string in `tzdata` and get hundreds or more of hits. :-( And I do believe we will get a better
code structure if the build team "owns" `make`; or at least has a vested interest in
what's in that directory. We still suffer a lot of the old "I don't know where to put this
file, so I'll just put it in make cause nobody cares about it anyway" mentality, but I've been
working for quite some time to make that list of misplaced files shorter and shorter.
I strongly agree with Magnus for all the same reasons. To me, the data
files are clearly a form of source code that should be considered owned
by the component teams. I'm honestly perplexed over why this is being
argued against.
/Erik
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 08:32:28 GMT, Magnus Ihse Bursie wrote: >> @mlchung If you don't have any strong preference, I implore you to accept >> this change. I **vastly** prefer to move the data out of `make`! >> >> This is not just about Skara tooling. When working with make files, autoconf >> and shell scripts, there is no fancy IDE support, so you are stuck with >> simple text editors and tools like `grep`. I've lost count on how many times >> I've had my grep searches blow up, since I happened to find e.g. a string in >> `tzdata` and get hundreds or more of hits. :-( And I do believe we will get >> a better code structure if the build team "owns" `make`; or at least has a >> vested interest in what's in that directory. We still suffer a lot of the >> old "I don't know where to put this file, so I'll just put it in make cause >> nobody cares about it anyway" mentality, but I've been working for quite >> some time to make that list of misplaced files shorter and shorter. > > Also, to clarify, for me there is a fundamental difference between > `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files > that are part of the module, owned by the content team, and the `$MODULE` > part is essential to delineate this content. The latter is owned by the build > team, and is just a convenient way to organize the build system within the > `make` directory. So it's clearly a no-no to put anything but `.gmk` files in > `make/modules/$MODULE`. The mapping and nr tables, and the *-X.java.template files in make/data/charsetmapping are used to generate source code for the java.base and jdk.charsets modules. The stdcs-$OS files configure the package and module that each charset go into. If the tables used to generate the source files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk will probably need a new home too. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 08:27:16 GMT, Magnus Ihse Bursie wrote:
>> Hi Magnus,
>>
>> I see the motivation of moving these build files for better identification
>> of ownership. Placing them under
>> `src/$MODULE/{share,$OS}/data` is one option. Given that skara will
>> automatically determine appropriate mailing lists of a PR, it seems that
>> `make/modules/$MODULE/data` can be another alternative that skara can
>> include this pattern in the mailing list configuration?? I don't yet have
>> a strong preference while I don't consider everything under `make` must be
>> owned by the build team though. Do you see one option is better than the
>> other?
>
> @mlchung If you don't have any strong preference, I implore you to accept
> this change. I **vastly** prefer to move the data out of `make`!
>
> This is not just about Skara tooling. When working with make files, autoconf
> and shell scripts, there is no fancy IDE support, so you are stuck with
> simple text editors and tools like `grep`. I've lost count on how many times
> I've had my grep searches blow up, since I happened to find e.g. a string in
> `tzdata` and get hundreds or more of hits. :-( And I do believe we will get a
> better code structure if the build team "owns" `make`; or at least has a
> vested interest in what's in that directory. We still suffer a lot of the old
> "I don't know where to put this file, so I'll just put it in make cause
> nobody cares about it anyway" mentality, but I've been working for quite some
> time to make that list of misplaced files shorter and shorter.
Also, to clarify, for me there is a fundamental difference between
`src/$MODULE` and `make/modules/$MODULE`. The former is the home of files that
are part of the module, owned by the content team, and the `$MODULE` part is
essential to delineate this content. The latter is owned by the build team, and
is just a convenient way to organize the build system within the `make`
directory. So it's clearly a no-no to put anything but `.gmk` files in
`make/modules/$MODULE`.
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 02:40:43 GMT, Mandy Chung wrote:
>> I have reviewed all lines in the patch file with or near instances of
>> `jdk.compiler`
>
> Hi Magnus,
>
> I see the motivation of moving these build files for better identification of
> ownership. Placing them under
> `src/$MODULE/{share,$OS}/data` is one option. Given that skara will
> automatically determine appropriate mailing lists of a PR, it seems that
> `make/modules/$MODULE/data` can be another alternative that skara can include
> this pattern in the mailing list configuration?? I don't yet have a strong
> preference while I don't consider everything under `make` must be owned by
> the build team though. Do you see one option is better than the other?
@mlchung If you don't have any strong preference, I implore you to accept this
change. I **vastly** prefer to move the data out of `make`!
This is not just about Skara tooling. When working with make files, autoconf
and shell scripts, there is no fancy IDE support, so you are stuck with simple
text editors and tools like `grep`. I've lost count on how many times I've had
my grep searches blow up, since I happened to find e.g. a string in `tzdata`
and get hundreds or more of hits. :-( And I do believe we will get a better
code structure if the build team "owns" `make`; or at least has a vested
interest in what's in that directory. We still suffer a lot of the old "I don't
know where to put this file, so I'll just put it in make cause nobody cares
about it anyway" mentality, but I've been working for quite some time to make
that list of misplaced files shorter and shorter.
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Mon, 7 Dec 2020 19:31:59 GMT, Jonathan Gibbons 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
>
> I have reviewed all lines in the patch file with or near instances of
> `jdk.compiler`
Hi Magnus,
I see the motivation of moving these build files for better identification of
ownership. Placing them under
`src/$MODULE/{share,$OS}/data` is one option. Given that skara will
automatically determine appropriate mailing lists of a PR, it seems that
`make/modules/$MODULE/data` can be another alternative that skara can include
this pattern in the mailing list configuration?? I don't yet have a strong
preference while I don't consider everything under `make` must be owned by the
build team though. Do you see one option is better than the other?
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Mon, 7 Dec 2020 14:27:45 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. > > 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 I have reviewed all lines in the patch file with or near instances of `jdk.compiler` - Marked as reviewed by jjg (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
> 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. 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1611/files - new: https://git.openjdk.java.net/jdk/pull/1611/files/8e775e40..f0047704 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1611=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1611=00-01 Stats: 56 lines in 1565 files changed: 1 ins; 0 del; 55 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
