Re: RFR: 8296478: Rework 8282948 and 8282700 to use the new autoconf UTIL_ARG_WITH [v5]
On Fri, 18 Nov 2022 11:27:14 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Uh oh > > Aaaahhh, **that** bug. I've run into that before. I did attempt to fix it but > after wasting too many hours I gave up. :-( It seems to be a limitation in m4 > that I cannot understand how to get around. At some point, the string > literal, even though quoted inside the `[...]`, is expanded and parsed as m4 > macro expansion arguments. In theory, I should have been able to add an > additional layer of quoting and then un-quote it once it was past the > problematic point, but that did not work. > > I solved the problem by not solving it, and instead rephrased the message to > not need the comma. (Imho, this limitation actually improved the quality of > the descriptions, so it was not bad per se). > > But I agree that it is annoying to have such a limitation in > `UTIL_DEFUN_NAMED`. If you want to have a go at trying to solve it, please > do! I'll fully admit my shortcomings and state that trying to solve this > passes my knowledge and ability to manipulate m4. > > Or, maybe, you could add some documentation to `UTIL_DEFUN_NAMED` and > `UTIL_ARG_WITH`, saying that comma is not allowed in the values. @magicus is there something up with the Windows tests? They seem to suddenly not be able to handle the `build-user` option, while all other platforms don't have much of an issue. Weirder still is that both WSL and MSYS2 on my Windows device are both perfectly fine with the option, and are able to complete the configure step with no hassle whatsoever Failing tests: checking for --with-build-user... , default configure: Invalid value for --with-build-user: "" configure: --with-build-user cannot be empty WSL and MSYS on my end: checking for --with-build-user... vertig0, default checking for --with-jdk-rc-name... OpenJDK Platform, default checking for --with-vendor-name... N/A, default checking for --with-vendor-url... https://openjdk.org/, default checking for --with-vendor-bug-url... https://bugreport.java.com/bugreport/, default checking for --with-vendor-vm-bug-url... https://bugreport.java.com/bugreport/crash.jsp, default checking for --with-version-string... , default checking for --with-version-feature... 20, default checking for --with-version-date... 2023-03-21, default checking for --with-vendor-version-string... , default checking for --with-macosx-bundle-name-base... OpenJDK, default checking for --with-macosx-bundle-id-base... net.java.openjdk-internal, default checking for --with-macosx-bundle-build-version... 0, default checking for version string... 20-internal-adhoc.vertig0.jdk - PR: https://git.openjdk.org/jdk/pull/11020
Re: RFR: 8296478: Rework 8282948 and 8282700 to use the new autoconf UTIL_ARG_WITH [v5]
On Fri, 18 Nov 2022 11:27:14 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Uh oh > > Aaaahhh, **that** bug. I've run into that before. I did attempt to fix it but > after wasting too many hours I gave up. :-( It seems to be a limitation in m4 > that I cannot understand how to get around. At some point, the string > literal, even though quoted inside the `[...]`, is expanded and parsed as m4 > macro expansion arguments. In theory, I should have been able to add an > additional layer of quoting and then un-quote it once it was past the > problematic point, but that did not work. > > I solved the problem by not solving it, and instead rephrased the message to > not need the comma. (Imho, this limitation actually improved the quality of > the descriptions, so it was not bad per se). > > But I agree that it is annoying to have such a limitation in > `UTIL_DEFUN_NAMED`. If you want to have a go at trying to solve it, please > do! I'll fully admit my shortcomings and state that trying to solve this > passes my knowledge and ability to manipulate m4. > > Or, maybe, you could add some documentation to `UTIL_DEFUN_NAMED` and > `UTIL_ARG_WITH`, saying that comma is not allowed in the values. @magicus I need some of your input on this: Typically when using AC_ARG_WITH one would surround the if block with an extra quote, for instance if we had `if ! [[ $JDK_RC_NAME =~ ^[[:print:]]*$ ]]` the code in m4 would have an extra set of brackets to quote it so m4 doesn't absolutely wreck the final script that we want to emit, such as `if [ ! [[ $JDK_RC_NAME =~ ^[[:print:]]*$ ]] ]` I'm about halfway through fixing the whole issue with commas, and I'm fairly certain I could lump the problem with quoting expansion in there too. My question though is: When it comes to parameters that take "predicate" code blocks as arguments (Such as CHECK_VALUE), should I have UTIL_ARG_NAMED handle the annoying issue of quoting entirely so the caller can freely pass `if ! [[ $JDK_RC_NAME =~ ^[[:print:]]*$ ]]` without having to worry at all, or should I make users still have to keep passing that extra level of quotation into things like IF_GIVEN to match what is already being done? - PR: https://git.openjdk.org/jdk/pull/11020
Re: RFR: 8296478: Rework 8282948 and 8282700 to use the new autoconf UTIL_ARG_WITH [v5]
On Fri, 18 Nov 2022 11:27:14 GMT, Magnus Ihse Bursie wrote: > Aaaahhh, **that** bug. I've run into that before. I did attempt to fix it but > after wasting too many hours I gave up. :-( It seems to be a limitation in m4 > that I cannot understand how to get around. At some point, the string > literal, even though quoted inside the `[...]`, is expanded and parsed as m4 > macro expansion arguments. In theory, I should have been able to add an > additional layer of quoting and then un-quote it once it was past the > problematic point, but that did not work. > > I solved the problem by not solving it, and instead rephrased the message to > not need the comma. (Imho, this limitation actually improved the quality of > the descriptions, so it was not bad per se). > > But I agree that it is annoying to have such a limitation in > `UTIL_DEFUN_NAMED`. If you want to have a go at trying to solve it, please > do! I'll fully admit my shortcomings and state that trying to solve this > passes my knowledge and ability to manipulate m4. > > Or, maybe, you could add some documentation to `UTIL_DEFUN_NAMED` and > `UTIL_ARG_WITH`, saying that comma is not allowed in the values. Oh dear, I was looking to you for help with that one :( I'll see if there's a way to solve this on my end, but I really doubt I'll be able to if even you couldn't... What I'm more worried about though is that this same issue also seems to be causing the problem I found when using CHECK_VALUE (see above) where it's actually outright deleting the square brackets in the code passed inside the block. I don't know why this didn't initially happen earlier when I re-ran configure with different options as per Erik's request, but I can verify that this affects all the "predicate" parameters of UTIL_ARG_WITH (IF_GIVEN, IF_NOT_GIVEN, etc). This is a way more serious problem that I doubt can be commented away with some documentation since it destroys several widely shell constructs that don't have any valid replacements, which is going to be pretty problematic Side tangent - Is my usage of DEFAULT_DESC correct? - PR: https://git.openjdk.org/jdk/pull/11020
Re: RFR: 8296478: Rework 8282948 and 8282700 to use the new autoconf UTIL_ARG_WITH [v5]
On Thu, 17 Nov 2022 06:36:50 GMT, Julian Waters wrote: >> 8285093 introduced the new UTIL_ARG_WITH definition, which was not available >> when both 8282948 and 8282700 were written. They can now be moved to using >> the cleaner logic that UTIL_ARG_WITH grants. >> >> There are many more options that still use AC_ARG_WITH in jdk-version.m4. >> They are out of the scope of this commit, which aims only to rework the >> previous integrated commits mentioned above. > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Uh oh Aaaahhh, **that** bug. I've run into that before. I did attempt to fix it but after wasting too many hours I gave up. :-( It seems to be a limitation in m4 that I cannot understand how to get around. At some point, the string literal, even though quoted inside the `[...]`, is expanded and parsed as m4 macro expansion arguments. In theory, I should have been able to add an additional layer of quoting and then un-quote it once it was past the problematic point, but that did not work. I solved the problem by not solving it, and instead rephrased the message to not need the comma. (Imho, this limitation actually improved the quality of the descriptions, so it was not bad per se). But I agree that it is annoying to have such a limitation in `UTIL_DEFUN_NAMED`. If you want to have a go at trying to solve it, please do! I'll fully admit my shortcomings and state that trying to solve this passes my knowledge and ability to manipulate m4. Or, maybe, you could add some documentation to `UTIL_DEFUN_NAMED` and `UTIL_ARG_WITH`, saying that comma is not allowed in the values. - PR: https://git.openjdk.org/jdk/pull/11020
Re: RFR: 8296478: Rework 8282948 and 8282700 to use the new autoconf UTIL_ARG_WITH [v5]
On Thu, 17 Nov 2022 06:36:50 GMT, Julian Waters wrote: >> 8285093 introduced the new UTIL_ARG_WITH definition, which was not available >> when both 8282948 and 8282700 were written. They can now be moved to using >> the cleaner logic that UTIL_ARG_WITH grants. >> >> There are many more options that still use AC_ARG_WITH in jdk-version.m4. >> They are out of the scope of this commit, which aims only to rework the >> previous integrated commits mentioned above. > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Uh oh I've found the reason after headbanging for quite a while: `DESC: [Set vendor name. Among others, used to set the 'java.vendor' and 'java.vm.vendor' system properties.]` It's the comma in between "Among others, used to set...". This seems like a general flaw in UTIL_ARG_WITH and (I believe?) more fundamentally in UTIL_DEFUN_NAMED. I could rewrite the message itself to not have the comma and bypass this entirely, but I'll refrain from doing so since I doubt you'd want this bug to continue persisting silently where it might end up rearing its ugly head when it's used again in future commits - PR: https://git.openjdk.org/jdk/pull/11020
Re: RFR: 8296478: Rework 8282948 and 8282700 to use the new autoconf UTIL_ARG_WITH [v5]
> 8285093 introduced the new UTIL_ARG_WITH definition, which was not available > when both 8282948 and 8282700 were written. They can now be moved to using > the cleaner logic that UTIL_ARG_WITH grants. > > There are many more options that still use AC_ARG_WITH in jdk-version.m4. > They are out of the scope of this commit, which aims only to rework the > previous integrated commits mentioned above. Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Uh oh - Changes: - all: https://git.openjdk.org/jdk/pull/11020/files - new: https://git.openjdk.org/jdk/pull/11020/files/239c88af..4cbd7207 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11020&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11020&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11020.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11020/head:pull/11020 PR: https://git.openjdk.org/jdk/pull/11020
