Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie wrote: >> This is the first step of several, in which I will clean up the native >> compilation code as used by modules. In this first step `java.base`, >> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since >> they require more work. The changes in the remaining modules are trivial by >> comparison. >> >> The changes done here are: >> >> 1) A new argument `JDK_LIB` has been introduced, that is used for linking >> with other libraries produced by the JDK build. As a follow-up, this will be >> further cleaned up and generalized, but the goal for this change is just to >> separate them out from external libraries. >> >> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in >> alphabetical order. Note that this change will affect the resulting binaries >> (since the order libraries are given are stored in the binary), but this >> change should only be superficial. (If we have symbol clashes between >> libraries, then we have problems on a whole other level...). >> >> 3) The code has been checked for inconsistencies and style guide errors, and >> a common programming style has been applied to all `Lib.gmk` and >> `Launcher.gmk` files, making sure that all parts follow best practices. >> >> This PR will be followed up by invidual PRs for the modules requiring not >> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and >> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across >> platforms, and automatically apply proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18430#pullrequestreview-1954897229
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 11:38:25 GMT, Magnus Ihse Bursie wrote: > I can give a spoiler to what the upcoming JDK_LIBS rewrite will do. > > Currently, if you want to link with e.g. `jli`, this is what you need to do: > > ``` > $(eval $(call SetupJdkLibrary, BUILD_LIBMYLIB, \ > NAME := mylib, \ > EXTRA_HEADER_DIRS := java.base:libjli, \ > LDFLAGS_linux := -L$(call FindLibDirForModule, java.base), \ > LDFLAGS_macosx := -L$(call FindLibDirForModule, java.base), \ > JDK_LIBS_linux := -ljli, \ > JDK_LIBS_macosx := -ljli, \ > JDK_LIBS_windows := $(SUPPORT_OUTPUTDIR)/native/java.base/libjli/jli.lib, > \ > )) > > $(BUILD_LIBMYLIB): $(call FindLib, java.base, jli) > ``` > > This is cumbersome, and easy to forget (we almost never get it 100% right in > any place...). > > I intend to replace it with the following, and to have the build system > generate the boiler plate automatically: > > ``` > $(eval $(call SetupJdkLibrary, BUILD_LIBMYLIB, \ > NAME := mylib, \ > JDK_LIBS := jli, \ > )) > ``` I see, that's a definite +1 for me - PR Comment: https://git.openjdk.org/jdk/pull/18430#issuecomment-2014905990
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie wrote: >> This is the first step of several, in which I will clean up the native >> compilation code as used by modules. In this first step `java.base`, >> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since >> they require more work. The changes in the remaining modules are trivial by >> comparison. >> >> The changes done here are: >> >> 1) A new argument `JDK_LIB` has been introduced, that is used for linking >> with other libraries produced by the JDK build. As a follow-up, this will be >> further cleaned up and generalized, but the goal for this change is just to >> separate them out from external libraries. >> >> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in >> alphabetical order. Note that this change will affect the resulting binaries >> (since the order libraries are given are stored in the binary), but this >> change should only be superficial. (If we have symbol clashes between >> libraries, then we have problems on a whole other level...). >> >> 3) The code has been checked for inconsistencies and style guide errors, and >> a common programming style has been applied to all `Lib.gmk` and >> `Launcher.gmk` files, making sure that all parts follow best practices. >> >> This PR will be followed up by invidual PRs for the modules requiring not >> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and >> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across >> platforms, and automatically apply proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation I can give a spoiler to what the upcoming JDK_LIBS rewrite will do. Currently, if you want to link with e.g. `jli`, this is what you need to do: $(eval $(call SetupJdkLibrary, BUILD_LIBMYLIB, \ NAME := mylib, \ EXTRA_HEADER_DIRS := java.base:libjli, \ LDFLAGS_linux := -L$(call FindLibDirForModule, java.base), \ LDFLAGS_macosx := -L$(call FindLibDirForModule, java.base), \ JDK_LIBS_linux := -ljli, \ JDK_LIBS_macosx := -ljli, \ JDK_LIBS_windows := $(SUPPORT_OUTPUTDIR)/native/java.base/libjli/jli.lib, \ )) $(BUILD_LIBMYLIB): $(call FindLib, java.base, jli) This is cumbersome, and easy to forget (we almost never get it 100% right in any place...). I intend to replace it with the following, and to have the build system generate the boiler plate automatically: $(eval $(call SetupJdkLibrary, BUILD_LIBMYLIB, \ NAME := mylib, \ JDK_LIBS := jli, \ )) - PR Comment: https://git.openjdk.org/jdk/pull/18430#issuecomment-2014900723
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:47:30 GMT, Julian Waters wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix indentation > > make/common/JdkNativeCompilation.gmk line 190: > >> 188: $1_SRC_HEADER_FLAGS += $$(addprefix -I, $$(call GetJavaHeaderDir, >> $$(MODULE))) >> 189: >> 190: $1_JDK_LIBS += $$($1_JDK_LIBS_$$(OPENJDK_TARGET_OS)) > > Should these follow LIBS and pick up more specific target options as well? > Not a request, just thinking out loud I'm not sure what targets you are thinking of here. This is one of the cases where the distinction between OS and toolchain is not entirely clear. I'm treating them as synonyms. There are two reasons two separate target OS here: 1) syntax in linker. The microsoft linker uses `jvm.lib`, all others `-ljvm`. 2) different needs in the source code. When there are vastly different implementations for a lib on different OSes, we can have a situation where e.g. the macOS port needs libjava, but the other platforms don't. Issue 1) will be resolved by the upcoming JDK_LIBS rewrite. Issue 2 is inherited from the limitations in the source code, and since that source code is structured on a per-OS basis, we will only need to have JDK_LIBS options per-OS. - PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1535437421
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:43:40 GMT, Julian Waters wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix indentation > > make/autoconf/libraries.m4 line 174: > >> 172: >> 173: JDKLIB_LIBS="$BASIC_JDKLIB_LIBS" >> 174: JDKEXE_LIBS="" > > This is passed to LauncherCommon while JDKLIB_LIBS is manually passed to > every individual library it's used for. Why not unify them and pass > JDKLIB_LIBS to LibCommon instead? Other than that seems good JDKLIB_LIBS will be removed when I finish up the JDK_LIBS patch. (It is currently "-ljava -ljvm" on non-Windows, so basically a shortcut for setting dependencies on some commonly used JDK libraries.) JDKEXE_LIBS was just dead code, so I removed it "eagerly" in this patch. - PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1535433200
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie wrote: >> This is the first step of several, in which I will clean up the native >> compilation code as used by modules. In this first step `java.base`, >> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since >> they require more work. The changes in the remaining modules are trivial by >> comparison. >> >> The changes done here are: >> >> 1) A new argument `JDK_LIB` has been introduced, that is used for linking >> with other libraries produced by the JDK build. As a follow-up, this will be >> further cleaned up and generalized, but the goal for this change is just to >> separate them out from external libraries. >> >> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in >> alphabetical order. Note that this change will affect the resulting binaries >> (since the order libraries are given are stored in the binary), but this >> change should only be superficial. (If we have symbol clashes between >> libraries, then we have problems on a whole other level...). >> >> 3) The code has been checked for inconsistencies and style guide errors, and >> a common programming style has been applied to all `Lib.gmk` and >> `Launcher.gmk` files, making sure that all parts follow best practices. >> >> This PR will be followed up by invidual PRs for the modules requiring not >> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and >> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across >> platforms, and automatically apply proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation make/common/JdkNativeCompilation.gmk line 190: > 188: $1_SRC_HEADER_FLAGS += $$(addprefix -I, $$(call GetJavaHeaderDir, > $$(MODULE))) > 189: > 190: $1_JDK_LIBS += $$($1_JDK_LIBS_$$(OPENJDK_TARGET_OS)) Should these follow LIBS and pick up more specific target options as well? Not a request, just thinking out loud - PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1535391514
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie wrote: >> This is the first step of several, in which I will clean up the native >> compilation code as used by modules. In this first step `java.base`, >> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since >> they require more work. The changes in the remaining modules are trivial by >> comparison. >> >> The changes done here are: >> >> 1) A new argument `JDK_LIB` has been introduced, that is used for linking >> with other libraries produced by the JDK build. As a follow-up, this will be >> further cleaned up and generalized, but the goal for this change is just to >> separate them out from external libraries. >> >> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in >> alphabetical order. Note that this change will affect the resulting binaries >> (since the order libraries are given are stored in the binary), but this >> change should only be superficial. (If we have symbol clashes between >> libraries, then we have problems on a whole other level...). >> >> 3) The code has been checked for inconsistencies and style guide errors, and >> a common programming style has been applied to all `Lib.gmk` and >> `Launcher.gmk` files, making sure that all parts follow best practices. >> >> This PR will be followed up by invidual PRs for the modules requiring not >> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and >> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across >> platforms, and automatically apply proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation I had doubts about introducing JDK_LIB until I saw there were plans to further improve upon it, so I guess my uncertainty is now answered make/autoconf/libraries.m4 line 174: > 172: > 173: JDKLIB_LIBS="$BASIC_JDKLIB_LIBS" > 174: JDKEXE_LIBS="" This is passed to LauncherCommon while JDKLIB_LIBS is manually passed to every individual library it's used for. Why not unify them and pass JDKLIB_LIBS to LibCommon instead? Other than that seems good - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/18430#pullrequestreview-1954491578 PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1535387243
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
> This is the first step of several, in which I will clean up the native > compilation code as used by modules. In this first step `java.base`, > `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since > they require more work. The changes in the remaining modules are trivial by > comparison. > > The changes done here are: > > 1) A new argument `JDK_LIB` has been introduced, that is used for linking > with other libraries produced by the JDK build. As a follow-up, this will be > further cleaned up and generalized, but the goal for this change is just to > separate them out from external libraries. > > 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in > alphabetical order. Note that this change will affect the resulting binaries > (since the order libraries are given are stored in the binary), but this > change should only be superficial. (If we have symbol clashes between > libraries, then we have problems on a whole other level...). > > 3) The code has been checked for inconsistencies and style guide errors, and > a common programming style has been applied to all `Lib.gmk` and > `Launcher.gmk` files, making sure that all parts follow best practices. > > This PR will be followed up by invidual PRs for the modules requiring not > jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and > `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across > platforms, and automatically apply proper dependencies. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Fix indentation - Changes: - all: https://git.openjdk.org/jdk/pull/18430/files - new: https://git.openjdk.org/jdk/pull/18430/files/b26b59f1..f68c4a29 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18430&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18430&range=00-01 Stats: 7 lines in 3 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/18430.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18430/head:pull/18430 PR: https://git.openjdk.org/jdk/pull/18430
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation
On Thu, 21 Mar 2024 17:56:12 GMT, Erik Joelsson wrote: > Looks good, just found some indentation levels not conforming to the > convention and as you are fixing style, I pointed them out. Absolutely! Fixing style was one of the points here. It's funny, really, I've been staring at this code until my eyes started bleeding, and I could still not see these problems. - PR Comment: https://git.openjdk.org/jdk/pull/18430#issuecomment-2014756999
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation
On Thu, 21 Mar 2024 11:49:11 GMT, Magnus Ihse Bursie wrote: > This is the first step of several, in which I will clean up the native > compilation code as used by modules. In this first step `java.base`, > `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since > they require more work. The changes in the remaining modules are trivial by > comparison. > > The changes done here are: > > 1) A new argument `JDK_LIB` has been introduced, that is used for linking > with other libraries produced by the JDK build. As a follow-up, this will be > further cleaned up and generalized, but the goal for this change is just to > separate them out from external libraries. > > 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in > alphabetical order. Note that this change will affect the resulting binaries > (since the order libraries are given are stored in the binary), but this > change should only be superficial. (If we have symbol clashes between > libraries, then we have problems on a whole other level...). > > 3) The code has been checked for inconsistencies and style guide errors, and > a common programming style has been applied to all `Lib.gmk` and > `Launcher.gmk` files, making sure that all parts follow best practices. > > This PR will be followed up by invidual PRs for the modules requiring not > jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and > `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across > platforms, and automatically apply proper dependencies. Looks good, just found some indentation levels not conforming to the convention and as you are fixing style, I pointed them out. make/common/modules/LauncherCommon.gmk line 159: > 157: JDK_LIBS_windows := \ > 158: $(SUPPORT_OUTPUTDIR)/native/java.base/libjava/java.lib \ > 159: $$($1_WINDOWS_JLI_LIB), \ Indentation 4 make/modules/jdk.compiler/Launcher.gmk line 35: > 33:MAIN_CLASS := com.sun.tools.javac.Main, \ > 34:JAVA_ARGS := --add-modules ALL-DEFAULT, \ > 35:CFLAGS := -DEXPAND_CLASSPATH_WILDCARDS, \ Since you are in the area, indentation 4. make/modules/jdk.jdwp.agent/Lib.gmk line 64: > 62: EXTRA_HEADER_DIRS := \ > 63: include \ > 64: libjdwp/export, \ Indentation 4 - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18430#pullrequestreview-1953003346 PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1534366325 PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1534374945 PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1534379077
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation
On Thu, 21 Mar 2024 11:49:11 GMT, Magnus Ihse Bursie wrote: > This is the first step of several, in which I will clean up the native > compilation code as used by modules. In this first step `java.base`, > `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since > they require more work. The changes in the remaining modules are trivial by > comparison. > > The changes done here are: > > 1) A new argument `JDK_LIB` has been introduced, that is used for linking > with other libraries produced by the JDK build. As a follow-up, this will be > further cleaned up and generalized, but the goal for this change is just to > separate them out from external libraries. > > 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in > alphabetical order. Note that this change will affect the resulting binaries > (since the order libraries are given are stored in the binary), but this > change should only be superficial. (If we have symbol clashes between > libraries, then we have problems on a whole other level...). > > 3) The code has been checked for inconsistencies and style guide errors, and > a common programming style has been applied to all `Lib.gmk` and > `Launcher.gmk` files, making sure that all parts follow best practices. > > This PR will be followed up by invidual PRs for the modules requiring not > jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and > `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across > platforms, and automatically apply proper dependencies. I have checked this patch using COMPARE_BUILD on Oracle CI standard platforms. As expected, there are binary differences. I have looked through the binary differences, and to the best of my ability to judge, they are all effects of the change in library link order, or downstream changes as an effect of that primary change. This should have no actual runtime effect on the product. - PR Comment: https://git.openjdk.org/jdk/pull/18430#issuecomment-2012441544
RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation
This is the first step of several, in which I will clean up the native compilation code as used by modules. In this first step `java.base`, `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since they require more work. The changes in the remaining modules are trivial by comparison. The changes done here are: 1) A new argument `JDK_LIB` has been introduced, that is used for linking with other libraries produced by the JDK build. As a follow-up, this will be further cleaned up and generalized, but the goal for this change is just to separate them out from external libraries. 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in alphabetical order. Note that this change will affect the resulting binaries (since the order libraries are given are stored in the binary), but this change should only be superficial. (If we have symbol clashes between libraries, then we have problems on a whole other level...). 3) The code has been checked for inconsistencies and style guide errors, and a common programming style has been applied to all `Lib.gmk` and `Launcher.gmk` files, making sure that all parts follow best practices. This PR will be followed up by invidual PRs for the modules requiring not jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across platforms, and automatically apply proper dependencies. - Commit messages: - Update copyright years - 8328680: Introduce JDK_LIB, and clean up module native compilation Changes: https://git.openjdk.org/jdk/pull/18430/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18430&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8328680 Stats: 408 lines in 41 files changed: 211 ins; 73 del; 124 mod Patch: https://git.openjdk.org/jdk/pull/18430.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18430/head:pull/18430 PR: https://git.openjdk.org/jdk/pull/18430
