Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 12:42:09 GMT, Julian Waters wrote: >> It's in make/conf/branding.conf. > > I don't see a VENDOR_VERSION_STRING in there though, all vendor related > defaults I could find are > COMPANY_NAME=N/A > VENDOR_URL=https://openjdk.java.net/ > VENDOR_URL_BUG=https://bugreport.java.com/bugreport/ > VENDOR_URL_VM_BUG=https://bugreport.java.com/bugreport/crash.jsp Ah, sorry, you are right. I mixed it up with COMPANY_NAME. Yes, VENDOR_VERSION_STRING is allowed to be empty, so your code is correct. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 11:54:13 GMT, Magnus Ihse Bursie wrote: >> I treated this differently since it didn't seem to have any default value at >> all, as opposed to the other vendor options. I may have made a mistake while >> looking for where the vendor version string is defined in this case > > It's in make/conf/branding.conf. I don't see a VENDOR_VERSION_STRING in there though, all vendor related defaults I could find are COMPANY_NAME=N/A VENDOR_URL=https://openjdk.java.net/ VENDOR_URL_BUG=https://bugreport.java.com/bugreport/ VENDOR_URL_VM_BUG=https://bugreport.java.com/bugreport/crash.jsp - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 11:57:13 GMT, Magnus Ihse Bursie wrote: >> Got it, should I link JDK-8282948 into this PR while addressing this issue >> in the meantime? >> (On a side note the macOS tier 1 tests are also failing after this change, >> so you may be right that the build will inevitably fail when this has no >> value) > > JDK-8282948 will go in before this. But you can fix your code in the meantime > to do the same yes/no check. alright - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 10:31:17 GMT, Julian Waters wrote: >> make/autoconf/jdk-version.m4 line 507: >> >>> 505: # Set vendor version string if --without is not passed >>> 506: # Check not required if an empty value is passed, since >>> VENDOR_VERSION_STRING >>> 507: # would then be set to "" >> >> I'm not sure that works. In general, values like these are *supposed* to >> have a value. The default fallback is "N/A", which is not the empty string. >> I see no point in allowing overriding this to be empty. It just introduces >> cases where the build might fail later, for something that will hardly ever >> be tested. > > I treated this differently since it didn't seem to have any default value at > all, as opposed to the other vendor options. I may have made a mistake while > looking for where the vendor version string is defined in this case It's in make/conf/branding.conf. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 11:49:14 GMT, Julian Waters wrote: >> Ok, now I've tried to dig through Apple's somewhat confusing documentation >> on CFBundleVersion and CFBundleShortVersionString. My takeaway is that we >> should either publish the build number there, or 0 if we have no build >> number. >> >> This is basically what is done by the code, and that was *exactly* what was >> done by the code, prior to the fix were we allowed an empty build number >> (JDK-8274980). So this code should also have been updated then. I've opened >> JDK-8282948 to fix this. >> >> But still the effect for this PR is that a valueless >> --with-macosx-bundle-build-version is not accepted. > > Got it, should I link JDK-8282948 into this PR while addressing this issue in > the meantime? > (On a side note the macOS tier 1 tests are also failing after this change, so > you may be right that the build will inevitably fail when this has no value) JDK-8282948 will go in before this. But you can fix your code in the meantime to do the same yes/no check. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 11:42:05 GMT, Magnus Ihse Bursie wrote: >> Both version build and version opt have valid --without options though, if >> both are set (As in the case of an adhoc build run with >> --without-version-opt) that could be problematic, since the issue wouldn't >> be stemming from a bug in that case. I'll double check just to be sure > > Ok, now I've tried to dig through Apple's somewhat confusing documentation on > CFBundleVersion and CFBundleShortVersionString. My takeaway is that we should > either publish the build number there, or 0 if we have no build number. > > This is basically what is done by the code, and that was *exactly* what was > done by the code, prior to the fix were we allowed an empty build number > (JDK-8274980). So this code should also have been updated then. I've opened > JDK-8282948 to fix this. > > But still the effect for this PR is that a valueless > --with-macosx-bundle-build-version is not accepted. Got it, should I link JDK-8282948 into this PR while addressing this issue in the meantime? (On a side note the macOS tier 1 tests are also failing after this change, so you may be right that the build will inevitably fail when this has no value) - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 10:28:30 GMT, Julian Waters wrote: >> That might be. If that is so, then you've most likely spotted a bug. In >> practice, you're either building an official build and have a build number, >> or an adhoc build with an opt string, so I'm not suprised noone has run into >> this. > > Both version build and version opt have valid --without options though, if > both are set (As in the case of an adhoc build run with > --without-version-opt) that could be problematic, since the issue wouldn't be > stemming from a bug in that case. I'll double check just to be sure Ok, now I've tried to dig through Apple's somewhat confusing documentation on CFBundleVersion and CFBundleShortVersionString. My takeaway is that we should either publish the build number there, or 0 if we have no build number. This is basically what is done by the code, and that was *exactly* what was done by the code, prior to the fix were we allowed an empty build number (JDK-8274980). So this code should also have been updated then. I've opened JDK-8282948 to fix this. But still the effect for this PR is that a valueless --with-macosx-bundle-build-version is not accepted. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 09:40:13 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix syntax errors > > make/autoconf/jdk-version.m4 line 507: > >> 505: # Set vendor version string if --without is not passed >> 506: # Check not required if an empty value is passed, since >> VENDOR_VERSION_STRING >> 507: # would then be set to "" > > I'm not sure that works. In general, values like these are *supposed* to have > a value. The default fallback is "N/A", which is not the empty string. I see > no point in allowing overriding this to be empty. It just introduces cases > where the build might fail later, for something that will hardly ever be > tested. I treated this differently since it didn't seem to have any default value at all, as opposed to the other vendor options. I may have made a mistake while looking for where the vendor version string is defined in this case - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 09:58:29 GMT, Magnus Ihse Bursie wrote: >> From what I can tell, wouldn't MACOSX_BUNDLE_BUILD_VERSION be an empty >> string anyway if both VERSION_BUILD and VERSION_OPT = ""? > > That might be. If that is so, then you've most likely spotted a bug. In > practice, you're either building an official build and have a build number, > or an adhoc build with an opt string, so I'm not suprised noone has run into > this. Both version build and version opt have valid --without options though, if both are set (As in the case of an adhoc build run with --without-version-opt) that could be problematic, since the issue wouldn't be stemming from a bug in that case. I'll double check just to be sure - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 09:48:52 GMT, Julian Waters wrote: >> make/autoconf/jdk-version.m4 line 554: >> >>> 552: elif test "x$with_macosx_bundle_build_version" = xno; then >>> 553: # Interpret --without-* as empty string instead of the literal "no" >>> 554: MACOSX_BUNDLE_BUILD_VERSION= >> >> Same objection here. And even if it works right now, if we do allow this, it >> means that all future changes in the build system that involves >> MACOSX_BUNDLE_BUILD_VERSION needs to be verified that it works with the >> empty string. > > From what I can tell, wouldn't MACOSX_BUNDLE_BUILD_VERSION be an empty string > anyway if both VERSION_BUILD and VERSION_OPT = ""? That might be. If that is so, then you've most likely spotted a bug. In practice, you're either building an official build and have a build number, or an adhoc build with an opt string, so I'm not suprised noone has run into this. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 09:41:32 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix syntax errors > > make/autoconf/jdk-version.m4 line 554: > >> 552: elif test "x$with_macosx_bundle_build_version" = xno; then >> 553: # Interpret --without-* as empty string instead of the literal "no" >> 554: MACOSX_BUNDLE_BUILD_VERSION= > > Same objection here. And even if it works right now, if we do allow this, it > means that all future changes in the build system that involves > MACOSX_BUNDLE_BUILD_VERSION needs to be verified that it works with the empty > string. >From what I can tell, wouldn't MACOSX_BUNDLE_BUILD_VERSION be an empty string >anyway if both VERSION_BUILD and VERSION_OPT = ""? - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
On Thu, 10 Mar 2022 09:17:16 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: > > Fix syntax errors make/autoconf/jdk-version.m4 line 507: > 505: # Set vendor version string if --without is not passed > 506: # Check not required if an empty value is passed, since > VENDOR_VERSION_STRING > 507: # would then be set to "" I'm not sure that works. In general, values like these are *supposed* to have a value. The default fallback is "N/A", which is not the empty string. I see no point in allowing overriding this to be empty. It just introduces cases where the build might fail later, for something that will hardly ever be tested. make/autoconf/jdk-version.m4 line 554: > 552: elif test "x$with_macosx_bundle_build_version" = xno; then > 553: # Interpret --without-* as empty string instead of the literal "no" > 554: MACOSX_BUNDLE_BUILD_VERSION= Same objection here. And even if it works right now, if we do allow this, it means that all future changes in the build system that involves MACOSX_BUNDLE_BUILD_VERSION needs to be verified that it works with the empty string. - PR: https://git.openjdk.java.net/jdk/pull/7713
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v4]
> 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: Fix syntax errors - Changes: - all: https://git.openjdk.java.net/jdk/pull/7713/files - new: https://git.openjdk.java.net/jdk/pull/7713/files/18ec991d..52dd398e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7713=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7713=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 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
