Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Wed, 13 Mar 2024 09:49:13 GMT, Aleksey Shipilev wrote: >> make/modules/java.base/Launcher.gmk line 81: >> >>> 79: SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \ >>> 80: OPTIMIZATION := LOW, \ >>> 81: CFLAGS := $(CFLAGS_JDKEXE) \ >> >> There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing >> like flags we try to fill the line. >> Also, the indentation rules are that a broken line should be indented with >> four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk. > > My fault, I suggested Chad to do this. So what's the preferred formatting > here? Like BUILD_JEXEC block above does it? > > > CFLAGS := $(CFLAGS_JDKEXE) $(VERSION_CFLAGS) \ > -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava, \ Yeah, that looks good. I was thinking just appending it at the end like: CFLAGS := $(CFLAGS_JDKEXE) -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava \ $(VERSION_CFLAGS), \ but your version definitely looks better! - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522963660
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Tue, 12 Mar 2024 19:22:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with two additional > commits since the last revision: > > - Print to stdout instead of stderr > - Compare version using VERSION_STRING I was thinking what would happen for the first time we upgrade jspawnhelper like this. There would be no helpful message then, the `jspawnhelper` would just exit on argument count check. So I suggest we polish the failure messages a bit: always report version at `shutItDown`, and report more verbose failure messages too. See: [jspawnhelper-messages-1.patch](https://github.com/openjdk/jdk/files/14586196/jspawnhelper-messages-1.patch) Older JDK invoking new jspawnhelper would then fail like: % build/macosx-aarch64-server-fastdebug/images/jdk/lib/jspawnhelper 1:1:1 Incorrect number of arguments: 2 jspawnhelper version 23-internal-adhoc.shipilev.shipilev-jdk This command is not for general use and should only be run as the result of a call to ProcessBuilder.start() or Runtime.exec() in a java application - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1994051562
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Tue, 12 Mar 2024 23:44:45 GMT, Magnus Ihse Bursie wrote: >> Chad Rakoczy has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Print to stdout instead of stderr >> - Compare version using VERSION_STRING > > make/modules/java.base/Launcher.gmk line 81: > >> 79: SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \ >> 80: OPTIMIZATION := LOW, \ >> 81: CFLAGS := $(CFLAGS_JDKEXE) \ > > There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing > like flags we try to fill the line. > Also, the indentation rules are that a broken line should be indented with > four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk. My fault, I suggested Chad to do this. So what's the preferred formatting here? Like BUILD_JEXEC block above does it? CFLAGS := $(CFLAGS_JDKEXE) $(VERSION_CFLAGS) \ -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava, \ - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522886607
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Tue, 12 Mar 2024 19:22:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with two additional > commits since the last revision: > > - Print to stdout instead of stderr > - Compare version using VERSION_STRING make/modules/java.base/Launcher.gmk line 81: > 79: SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \ > 80: OPTIMIZATION := LOW, \ > 81: CFLAGS := $(CFLAGS_JDKEXE) \ There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing like flags we try to fill the line. Also, the indentation rules are that a broken line should be indented with four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk. - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522267500
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) > > Updates jspawnhelper to check that JDK version and jspawnhelper version are > the same. Updates test to include check for version. Also tested manually by > replacing jspawnhelper with incorrect version to confirm that check works. Chad Rakoczy has updated the pull request incrementally with two additional commits since the last revision: - Print to stdout instead of stderr - Compare version using VERSION_STRING - Changes: - all: https://git.openjdk.org/jdk/pull/18204/files - new: https://git.openjdk.org/jdk/pull/18204/files/7bbcc08f..62529391 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18204&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18204&range=01-02 Stats: 71 lines in 5 files changed: 12 ins; 41 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/18204.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18204/head:pull/18204 PR: https://git.openjdk.org/jdk/pull/18204
