Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Wed, 9 Mar 2022 04:36:13 GMT, Julian Waters wrote: >> This entire PR feels like a non-issue. >> >> I agree that the UX of the --with-* flags are not optimal, but I also doubt >> it worth putting much time and effort into fixing. Going through each and >> every --with-flag to determine the best way to handle the (almost always >> incorrect) --without-* variant is not something I'd look forward to. >> >> If anything should be done at all, I sincerely believe that merging "yes" >> and "no" will be the best way to handle. The fact that the error message >> mentions "--with" instead of "--without" is of no significance, since that's >> the way we treat the --with flags everywhere, just as the --enable/--disable >> dichotomy. > > I disagree with the assessment that this isn't an issue, since as I mentioned > above this can be fairly confusing, but I do concede that it would be tedious > to check every flag like this, so it seems merging yes and no is the way to > go. In that case though, I'm wondering if we should fold the x$with_foo = x > (ie --with-foo= ) into the yes and no check as well, since right now if > that's passed it assumes that the default option is requested. I could also > change the error message to be a generic one that matches both conditions, > but I'll leave that up for discussion for the time being > > As a side note, I'm also willing to be responsible for properly checking > every --without variant of all the current flags and any future ones, if it > ever comes to that Don't change the check for empty values. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Tue, 8 Mar 2022 14:02:35 GMT, Magnus Ihse Bursie wrote: >> I'm of the opinion that options which cannot have empty values, and will >> fall back to default ones if no explicit one is provided, would generate an >> error if --without-* is passed, while others that _can_ have empty values >> allow passing --without-* explicitly. From what I can gather so far, >> everything in branding.conf and a few other crucial options like >> --with-version-string or --with-version date are cases of the former, while >> --with-vendor-version-string would be an example of the latter. That said, >> it would be weird if we're folding no results into the yes branch, since >> passing --without-* would then return an error mentioning --with-* instead, >> which is in turn potentially confusing. Since we're on the topic, if this is >> the case, should -with-*= be considered as an error as well, for options >> that cannot have no value? (Currently it just assumes "Use the default >> value") > > This entire PR feels like a non-issue. > > I agree that the UX of the --with-* flags are not optimal, but I also doubt > it worth putting much time and effort into fixing. Going through each and > every --with-flag to determine the best way to handle the (almost always > incorrect) --without-* variant is not something I'd look forward to. > > If anything should be done at all, I sincerely believe that merging "yes" and > "no" will be the best way to handle. The fact that the error message mentions > "--with" instead of "--without" is of no significance, since that's the way > we treat the --with flags everywhere, just as the --enable/--disable > dichotomy. I disagree with the assessment that this isn't an issue, since as I mentioned above this can be fairly confusing, but I do concede that it would be tedious to check every flag like this, so it seems merging yes and no is the way to go. In that case though, I'm wondering if we should fold the x$with_foo = x (ie --with-foo= ) into the yes and no check as well, since right now if that's passed it assumes that the default option is requested. I could also change the error message to be a generic one that matches both conditions, but I'll leave that up for discussion for the time being As a side note, I'm also willing to be responsible for properly checking every --without variant of all the current flags and any future ones, if it ever comes to that - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Tue, 8 Mar 2022 02:48:45 GMT, Julian Waters wrote: >> ... and this goes for all the changes in the PR. > > I'm of the opinion that options which cannot have empty values, and will fall > back to default ones if no explicit one is provided, would generate an error > if --without-* is passed, while others that _can_ have empty values allow > passing --without-* explicitly. From what I can gather so far, everything in > branding.conf and a few other crucial options like --with-version-string or > --with-version date are cases of the former, while > --with-vendor-version-string would be an example of the latter. That said, it > would be weird if we're folding no results into the yes branch, since passing > --without-* would then return an error mentioning --with-* instead, which is > in turn potentially confusing. Since we're on the topic, if this is the case, > should -with-*= be considered as an error as well, for options that cannot > have no value? (Currently it just assumes "Use the default value") This entire PR feels like a non-issue. I agree that the UX of the --with-* flags are not optimal, but I also doubt it worth putting much time and effort into fixing. Going through each and every --with-flag to determine the best way to handle the (almost always incorrect) --without-* variant is not something I'd look forward to. If anything should be done at all, I sincerely believe that merging "yes" and "no" will be the best way to handle. The fact that the error message mentions "--with" instead of "--without" is of no significance, since that's the way we treat the --with flags everywhere, just as the --enable/--disable dichotomy. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Tue, 8 Mar 2022 04:15:51 GMT, David Holmes wrote: > Passing --without-x makes no sense at all IMHO. Even so, I'd argue that it should be handled properly and show what really went wrong, rather than throwing, for instance, "no is not a valid version string" (In the case of --without-version-string). Besides, to an outside observer some of the configure options that cannot have --without-* set would still seem to make sense when glanced over, it may not be immediately obvious that said option cannot have no value. Even without the errors though, many of the configure options also don't check if --without-* is passed, which would result in the silent build error of setting said option to a literal "no", that would appear to have worked correctly regardless (All the --with-vender-* options are examples of this) > If you pass "" as the value then you get what you deserve if the build fails. I've been wondering if I should fold that into the "yes" check for several of them, since many interpret --with-*= as "use the default value" - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Mon, 7 Mar 2022 08:07:30 GMT, Julian Waters wrote: >> Some of the --without options are not properly handled and will crash when >> processed (For example, --without-version-string), in other cases the >> --without-* option will actually silently produce incorrect results instead >> of actually doing what --without-* implies (For example, >> --without-build-user and all the --with-vendor-* options). The most elegant >> way to solve this would simply be to handle such cases and display warnings >> when they're encountered (or if the option is critical to the build process, >> throwing an error) >> >> Even if it doesn't make sense to pass said option however, it should display >> a warning instead of letting configure exit with a confusing error when it's >> run > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Handle remaining options and change critical options to error when > --without is passed I've never felt that configure's `--without-x` options really make any sense for the bulk of our `--with-x` options that define "properties". We have a bunch of properties that have to be set and the default is in a configuration file. You can then use `--with-x=foo` to override that. If you pass "" as the value then you get what you deserve if the build fails. Passing --without-x makes no sense at all IMHO. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Mon, 7 Mar 2022 19:54:17 GMT, Magnus Ihse Bursie wrote: >> I think this, even more, makes it clear that `--without-vendor-url` can´t >> possible be meant to be interpreted as "use https://openjdk.java.net/;. >> >> Basically, I think what I'm arguing for is that we can fold this check into >> the "yes" check -- both `--with-vendor-url` (with no given value) and >> `--without-vendor-url` are invalid. So something like this: >> >> >> if test "x$with_vendor_url" = xyes || test "x$with_vendor_url" = xno; then >> AC_MSG_ERROR([--with-vendor-url must have a value]) >> elif... > > ... and this goes for all the changes in the PR. I'm of the opinion that options which cannot have empty values, and will fall back to default ones if no explicit one is provided, would generate an error if --without-* is passed, while others that _can_ have empty values allow passing --without-* explicitly. From what I can gather so far, everything in branding.conf and a few other crucial options like --with-version-string or --with-version date are cases of the former, while --with-vendor-version-string would be an example of the latter. That said, it would be weird if we're folding no results into the yes branch, since passing --without-* would then return an error mentioning --with-* instead, which is in turn potentially confusing. Since we're on the topic, if this is the case, should -with-*= be considered as an error as well, for options that cannot have no value? (Currently it just assumes "Use the default value") - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Mon, 7 Mar 2022 19:53:34 GMT, Magnus Ihse Bursie wrote: >> make/autoconf/jdk-version.m4 line 129: >> >>> 127: AC_MSG_ERROR([--with-vendor-url must have a value]) >>> 128: elif test "x$with_vendor_url" = xno; then >>> 129: AC_MSG_WARN([--without-vendor-url is the same as not passing >>> --with-vendor-url to begin with]) >> >> Same as with vendor. > > I think this, even more, makes it clear that `--without-vendor-url` can´t > possible be meant to be interpreted as "use https://openjdk.java.net/;. > > Basically, I think what I'm arguing for is that we can fold this check into > the "yes" check -- both `--with-vendor-url` (with no given value) and > `--without-vendor-url` are invalid. So something like this: > > > if test "x$with_vendor_url" = xyes || test "x$with_vendor_url" = xno; then > AC_MSG_ERROR([--with-vendor-url must have a value]) > elif... ... and this goes for all the changes in the PR. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Mon, 7 Mar 2022 08:07:30 GMT, Julian Waters wrote: >> Some of the --without options are not properly handled and will crash when >> processed (For example, --without-version-string), in other cases the >> --without-* option will actually silently produce incorrect results instead >> of actually doing what --without-* implies (For example, >> --without-build-user and all the --with-vendor-* options). The most elegant >> way to solve this would simply be to handle such cases and display warnings >> when they're encountered (or if the option is critical to the build process, >> throwing an error) >> >> Even if it doesn't make sense to pass said option however, it should display >> a warning instead of letting configure exit with a confusing error when it's >> run > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Handle remaining options and change critical options to error when > --without is passed Changes requested by ihse (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Mon, 7 Mar 2022 13:57:42 GMT, Erik Joelsson wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Handle remaining options and change critical options to error when >> --without is passed > > make/autoconf/jdk-version.m4 line 113: > >> 111: AC_MSG_ERROR([--with-vendor-name must have a value]) >> 112: elif test "x$with_vendor_name" = xno; then >> 113: AC_MSG_WARN([--without-vendor-name is the same as not passing >> --with-vendor-name to begin with]) > > I don't think this warrants a warning. The user is just trying to be > explicit. It's good to handle the "no" case though. The entire semantics of the `--without-X` flags in configure is a bit unclear. If someone where to actually give `--without-vendor-name`, what would they suppose should happen? If it is left unspecified, the default value `N/A` is taken from branding.conf. But when setting `--without-vendor-name`, do you mean that you explicitly want the `N/A` value? Or do you mean that you want the build to not use a vendor name at all? In the latter case, that is not possible. And if we make that interpretation, this should not be a warning, but an error. To me it feel strange to set `--without-vendor-name` and assume that means "use the default value". I'd rather go with the interpretation that this is a misunderstanding of how the option works, and emit an error. Do you want to > make/autoconf/jdk-version.m4 line 129: > >> 127: AC_MSG_ERROR([--with-vendor-url must have a value]) >> 128: elif test "x$with_vendor_url" = xno; then >> 129: AC_MSG_WARN([--without-vendor-url is the same as not passing >> --with-vendor-url to begin with]) > > Same as with vendor. I think this, even more, makes it clear that `--without-vendor-url` can´t possible be meant to be interpreted as "use https://openjdk.java.net/;. Basically, I think what I'm arguing for is that we can fold this check into the "yes" check -- both `--with-vendor-url` (with no given value) and `--without-vendor-url` are invalid. So something like this: if test "x$with_vendor_url" = xyes || test "x$with_vendor_url" = xno; then AC_MSG_ERROR([--with-vendor-url must have a value]) elif... - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Mon, 7 Mar 2022 08:07:30 GMT, Julian Waters wrote: >> Some of the --without options are not properly handled and will crash when >> processed (For example, --without-version-string), in other cases the >> --without-* option will actually silently produce incorrect results instead >> of actually doing what --without-* implies (For example, >> --without-build-user and all the --with-vendor-* options). The most elegant >> way to solve this would simply be to handle such cases and display warnings >> when they're encountered (or if the option is critical to the build process, >> throwing an error) >> >> Even if it doesn't make sense to pass said option however, it should display >> a warning instead of letting configure exit with a confusing error when it's >> run > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Handle remaining options and change critical options to error when > --without is passed I think it's good to error out when incorrectly using --without-foo options. I don't think we need warnings in cases where --without-foo is equivalent to --with-foo="". make/autoconf/jdk-version.m4 line 113: > 111: AC_MSG_ERROR([--with-vendor-name must have a value]) > 112: elif test "x$with_vendor_name" = xno; then > 113: AC_MSG_WARN([--without-vendor-name is the same as not passing > --with-vendor-name to begin with]) I don't think this warrants a warning. The user is just trying to be explicit. It's good to handle the "no" case though. make/autoconf/jdk-version.m4 line 129: > 127: AC_MSG_ERROR([--with-vendor-url must have a value]) > 128: elif test "x$with_vendor_url" = xno; then > 129: AC_MSG_WARN([--without-vendor-url is the same as not passing > --with-vendor-url to begin with]) Same as with vendor. make/autoconf/jdk-version.m4 line 145: > 143: AC_MSG_ERROR([--with-vendor-bug-url must have a value]) > 144: elif test "x$with_vendor_bug_url" = xno; then > 145: AC_MSG_WARN([--without-vendor-bug-url is the same as not passing > --with-vendor-bug-url to begin with]) Same as with vendor. make/autoconf/jdk-version.m4 line 161: > 159: AC_MSG_ERROR([--with-vendor-vm-bug-url must have a value]) > 160: elif test "x$with_vendor_vm_bug_url" = xno; then > 161: AC_MSG_WARN([--without-vendor-vm-bug-url is the same as not passing > --with-vendor-vm-bug-url to begin with]) Same as with vendor. make/autoconf/jdk-version.m4 line 180: > 178: AC_MSG_ERROR([--with-version-string must have a value]) > 179: elif test "x$with_version_string" = xno; then > 180: AC_MSG_WARN([--without-version-string is the same as not passing > --with-version-string to begin with]) Same as with vendor. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
> Some of the --without options are not properly handled and will crash when > processed (For example, --without-version-string), in other cases the > --without-* option will actually silently produce incorrect results instead > of actually doing what --without-* implies (For example, --without-build-user > and all the --with-vendor-* options). The most elegant way to solve this > would simply be to handle such cases and display warnings when they're > encountered (or if the option is critical to the build process, throwing an > error) > > Even if it doesn't make sense to pass said option however, it should display > a warning instead of letting configure exit with a confusing error when it's > run > > This is an initial change and I may have missed several options; I'll double > check and make sure in the meantime Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Handle remaining options and change critical options to error when --without is passed - Changes: - all: https://git.openjdk.java.net/jdk/pull/7713/files - new: https://git.openjdk.java.net/jdk/pull/7713/files/54bbfae5..c9699eb7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7713=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7713=00-01 Stats: 24 lines in 1 file changed: 12 ins; 10 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7713.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7713/head:pull/7713 PR: https://git.openjdk.java.net/jdk/pull/7713
