Re: RFR: 8254702: jpackage app launcher crashes on CentOS [v2]
On Mon, 1 Feb 2021 18:35:54 GMT, Alexey Semenyuk wrote: >>> "common" was perfectly enough until this change. Unfortunately we cant just >>> drop new C sources in "common" dir because we don't want them to be >>> compiled with g++. That is why need new common directory >>> (applauncherlibcommon) for C sources. >> >> We pick compiler based on file suffix, not directory, so it shouldn't matter >> where you put a .c file, it should always be compiled with gcc and .cpp >> files with g++. Which compiler is used to launch the linker can however >> differ. That's configured for each SetupNativeCompilation call with the >> different TOOLCHAIN settings. > > Erik, thank you for explanation. > > The launcher on Linux should not be linked with c++ runtime, that is why > TOOLCHAIN_DEFAULT is used as a value for TOOLCHAIN property in > BUILD_JPACKAGE_APPLAUNCHEREXE target on Linux. > > Will SetupNativeCompilation work if `TOOLCHAIN=TOOLCHAIN_DEFAULT` and there > are .cpp sources are in directories passed in `SRC` property of > SetupNativeCompilation? Will it try to compile these sources? If it will > ignore them and pick only .c files, that would be perfect. Reworked the fix to avoid creation of extra source directories and file renames. - PR: https://git.openjdk.java.net/jdk/pull/2320
Re: RFR: 8254702: jpackage app launcher crashes on CentOS [v2]
On 2021-02-01 10:38, Alexey Semenyuk wrote: On Mon, 1 Feb 2021 18:24:23 GMT, Erik Joelsson wrote: "common" was perfectly enough until this change. Unfortunately we cant just drop new C sources in "common" dir because we don't want them to be compiled with g++. That is why need new common directory (applauncherlibcommon) for C sources. I'll rename applauncherlibcommon in libapplaunchercommon and applauncherlib in libapplauncher in the next commit. "common" was perfectly enough until this change. Unfortunately we cant just drop new C sources in "common" dir because we don't want them to be compiled with g++. That is why need new common directory (applauncherlibcommon) for C sources. We pick compiler based on file suffix, not directory, so it shouldn't matter where you put a .c file, it should always be compiled with gcc and .cpp files with g++. Which compiler is used to launch the linker can however differ. That's configured for each SetupNativeCompilation call with the different TOOLCHAIN settings. Erik, thank you for explanation. The launcher on Linux should not be linked with c++ runtime, that is why TOOLCHAIN_DEFAULT is used at a value for TOOLCHAIN property in BUILD_JPACKAGE_APPLAUNCHEREXE target on Linux. Will SetupNativeCompilation work if `TOOLCHAIN=TOOLCHAIN_DEFAULT` and there are .cpp sources are in directories passed in `SRC` property of SetupNativeCompilation? Will it try to compile these sources? If it will ignore them and pick only .c files, that would be perfect. SetupNativeCompilation will by default include all src files found in any directory given to SRC, recursively. You can use EXCLUDES, EXCLUDE_FILES and EXCLUDE_PATTERN to exclude files or directories from SRC. You can also use EXTRA_FILES to pick specific files outside of any directory in SRC. Sorting files in separate directories or using EXCLUDE*/EXTRA_FILES are both possible and picking the right solution is mostly down to taste. /Erik - PR: https://git.openjdk.java.net/jdk/pull/2320
Re: RFR: 8254702: jpackage app launcher crashes on CentOS [v2]
On Mon, 1 Feb 2021 18:24:23 GMT, Erik Joelsson wrote: >> "common" was perfectly enough until this change. Unfortunately we cant just >> drop new C sources in "common" dir because we don't want them to be compiled >> with g++. That is why need new common directory (applauncherlibcommon) for C >> sources. >> >> I'll rename applauncherlibcommon in libapplaunchercommon and applauncherlib >> in libapplauncher in the next commit. > >> "common" was perfectly enough until this change. Unfortunately we cant just >> drop new C sources in "common" dir because we don't want them to be compiled >> with g++. That is why need new common directory (applauncherlibcommon) for C >> sources. > > We pick compiler based on file suffix, not directory, so it shouldn't matter > where you put a .c file, it should always be compiled with gcc and .cpp files > with g++. Which compiler is used to launch the linker can however differ. > That's configured for each SetupNativeCompilation call with the different > TOOLCHAIN settings. Erik, thank you for explanation. The launcher on Linux should not be linked with c++ runtime, that is why TOOLCHAIN_DEFAULT is used at a value for TOOLCHAIN property in BUILD_JPACKAGE_APPLAUNCHEREXE target on Linux. Will SetupNativeCompilation work if `TOOLCHAIN=TOOLCHAIN_DEFAULT` and there are .cpp sources are in directories passed in `SRC` property of SetupNativeCompilation? Will it try to compile these sources? If it will ignore them and pick only .c files, that would be perfect. - PR: https://git.openjdk.java.net/jdk/pull/2320
Re: RFR: 8254702: jpackage app launcher crashes on CentOS [v2]
On Mon, 1 Feb 2021 16:17:35 GMT, Alexey Semenyuk wrote: > "common" was perfectly enough until this change. Unfortunately we cant just > drop new C sources in "common" dir because we don't want them to be compiled > with g++. That is why need new common directory (applauncherlibcommon) for C > sources. We pick compiler based on file suffix, not directory, so it shouldn't matter where you put a .c file, it should always be compiled with gcc and .cpp files with g++. Which compiler is used to launch the linker can however differ. That's configured for each SetupNativeCompilation call with the different TOOLCHAIN settings. - PR: https://git.openjdk.java.net/jdk/pull/2320
Re: RFR: 8254702: jpackage app launcher crashes on CentOS [v2]
On Mon, 1 Feb 2021 12:19:56 GMT, Magnus Ihse Bursie wrote: >> Alexey Semenyuk has refreshed the contents of this pull request, and >> previous commits have been removed. The incremental views will show >> differences compared to the previous content of the PR. The pull request >> contains one new commit since the last revision: >> >> 8254702: jpackage app launcher crashes on CentOS > > Changes requested by ihse (Reviewer). "common" was perfectly enough until this change. Unfortunately we cant just drop new C sources in "common" dir because we don't want them to be compiled with g++. That is why need new common directory (applauncherlibcommon) for C sources. I'll rename applauncherlibcommon in libapplaunchercommon and applauncherlib in libapplauncher in the next commit. - PR: https://git.openjdk.java.net/jdk/pull/2320
Re: RFR: 8254702: jpackage app launcher crashes on CentOS [v2]
On Mon, 1 Feb 2021 12:16:09 GMT, Magnus Ihse Bursie wrote: >> Alexey Semenyuk has refreshed the contents of this pull request, and >> previous commits have been removed. The incremental views will show >> differences compared to the previous content of the PR. The pull request >> contains one new commit since the last revision: >> >> 8254702: jpackage app launcher crashes on CentOS > > make/modules/jdk.jpackage/Lib.gmk line 61: > >> 59: JPACKAGE_OUTPUT_DIR := >> $(JDK_OUTPUTDIR)/modules/$(MODULE)/jdk/jpackage/internal/resources >> 60: JPACKAGE_CXXFLAGS_windows := -EHsc -DUNICODE -D_UNICODE >> 61: JPACKAGE_CFLAGS_windows := -DUNICODE -D_UNICODE > > Why is this change modifying Windows? I thought it was supposed to be a > linux-only fix..? There is new shared JvmlLauncherLib.c file. This new make variable is to setup complier for this file on Windows. The functional change is Linux-only, however code base code reshuffled on all platforms. > make/modules/jdk.jpackage/Lib.gmk line 65: > >> 63: )) >> 64: >> 65: $(BUILD_JPACKAGE_APPLAUNCHEREXE): $(call FindLib, java.base, java) > > Why did you remove this dependency? I moved it to the bottom of the file making all artifacts produced by make/modules/jdk.jpackage/Lib.gmk depend on java.bas and java. There is `$(JPACKAGE_TARGETS): $(call FindLib, java.base, java)` at the bottom of the file. > make/modules/jdk.jpackage/Lib.gmk line 106: > >> 104: CFLAGS_linux := -Wno-format-nonliteral, \ >> 105: LDFLAGS := $(LDFLAGS_JDKLIB) \ >> 106: >> -Wl$(COMMA)--version-script=$(TOPDIR)/make/modules/$(MODULE)/applauncherlib.ld-version-script, >> \ > > We should really not be using linker scripts. I did not understand your > comment in the linker script -- was it only needed to handle your personal > build environment? If so, you need to fix your build environment instead. Yeh, I found that linker script is needed only in my local build env. I'll remove it then. - PR: https://git.openjdk.java.net/jdk/pull/2320
Re: RFR: 8254702: jpackage app launcher crashes on CentOS [v2]
On Fri, 29 Jan 2021 23:06:20 GMT, Alexey Semenyuk wrote: >> Fix for https://bugs.openjdk.java.net/browse/JDK-8254702 >> >> The fix splits Linux app launcher in app launcher and launcher shared lib. >> App launcher is pure C and doesn't have C++ code. App launcher lib >> incorporates bulk of C++ code from app launcher. >> At startup app launcher loads launcher shared lib and calls functions it >> provides to get data for launching JVM (path to jli lib and arguments for >> JLI_Launch function call). >> App launcher unloads launcher shared lib before launching JVM to remove C++ >> runtime from the process memory. >> >> Getting rid of C++ code from app launcher required to rewrite app >> installation location lookup code from C++ to C. LinuxPackage.c source is C >> alternative for >> https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Package.cpp >> and >> https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Executor.cpp. >> >> Layout of jpackage's native code changed: >> - `common`: code shared between all jpackage binaries. >> - `applauncher`: launcher only code. >> - `applauncherlib`: launcher lib code on Linux and launcher code on other >> platforms. >> - `applaunchercommon`: code shared between launcher and launcher lib on >> Linux and launcher code on other platforms. > > Alexey Semenyuk has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. This whole change seems very messy to me. :-( I'm having a hard time even untangling the PR to understand what's going on. Are you creating two new directories, "applauncherlib" and "applauncherlibcommon"? First of all, for shared libraries, the norm is to have a "lib-" prefix, not a "-lib" suffix. Secondly, there is already a "common" directory, is that not enough? Changes requested by ihse (Reviewer). src/jdk.jpackage/share/native/common/app.cpp line 26: > 24: */ > 25: > 26: #include "kludge_c++11.h" The name arose my curiosity, so I had to check out the file. Now that we indeed do have C++11 in the JDK (indeed, C++14), this should perhaps be revisited? (Not as part of this PR, of course) make/modules/jdk.jpackage/Lib.gmk line 61: > 59: JPACKAGE_OUTPUT_DIR := > $(JDK_OUTPUTDIR)/modules/$(MODULE)/jdk/jpackage/internal/resources > 60: JPACKAGE_CXXFLAGS_windows := -EHsc -DUNICODE -D_UNICODE > 61: JPACKAGE_CFLAGS_windows := -DUNICODE -D_UNICODE Why is this change modifying Windows? I thought it was supposed to be a linux-only fix..? make/modules/jdk.jpackage/Lib.gmk line 65: > 63: )) > 64: > 65: $(BUILD_JPACKAGE_APPLAUNCHEREXE): $(call FindLib, java.base, java) Why did you remove this dependency? make/modules/jdk.jpackage/Lib.gmk line 106: > 104: CFLAGS_linux := -Wno-format-nonliteral, \ > 105: LDFLAGS := $(LDFLAGS_JDKLIB) \ > 106: > -Wl$(COMMA)--version-script=$(TOPDIR)/make/modules/$(MODULE)/applauncherlib.ld-version-script, > \ We should really not be using linker scripts. I did not understand your comment in the linker script -- was it only needed to handle your personal build environment? If so, you need to fix your build environment instead. - PR: https://git.openjdk.java.net/jdk/pull/2320
Re: RFR: 8254702: jpackage app launcher crashes on CentOS [v2]
On Fri, 29 Jan 2021 23:06:20 GMT, Alexey Semenyuk wrote: >> Fix for https://bugs.openjdk.java.net/browse/JDK-8254702 >> >> The fix splits Linux app launcher in app launcher and launcher shared lib. >> App launcher is pure C and doesn't have C++ code. App launcher lib >> incorporates bulk of C++ code from app launcher. >> At startup app launcher loads launcher shared lib and calls functions it >> provides to get data for launching JVM (path to jli lib and arguments for >> JLI_Launch function call). >> App launcher unloads launcher shared lib before launching JVM to remove C++ >> runtime from the process memory. >> >> Getting rid of C++ code from app launcher required to rewrite app >> installation location lookup code from C++ to C. LinuxPackage.c source is C >> alternative for >> https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Package.cpp >> and >> https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Executor.cpp. >> >> Layout of jpackage's native code changed: >> - `common`: code shared between all jpackage binaries. >> - `applauncher`: launcher only code. >> - `applauncherlib`: launcher lib code on Linux and launcher code on other >> platforms. >> - `applaunchercommon`: code shared between launcher and launcher lib on >> Linux and launcher code on other platforms. > > Alexey Semenyuk has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. Marked as reviewed by almatvee (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2320
Re: RFR: 8254702: jpackage app launcher crashes on CentOS [v2]
> Fix for https://bugs.openjdk.java.net/browse/JDK-8254702 > > The fix splits Linux app launcher in app launcher and launcher shared lib. > App launcher is pure C and doesn't have C++ code. App launcher lib > incorporates bulk of C++ code from app launcher. > At startup app launcher loads launcher shared lib and calls functions it > provides to get data for launching JVM (path to jli lib and arguments for > JLI_Launch function call). > App launcher unloads launcher shared lib before launching JVM to remove C++ > runtime from the process memory. > > Getting rid of C++ code from app launcher required to rewrite app > installation location lookup code from C++ to C. LinuxPackage.c source is C > alternative for > https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Package.cpp > and > https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Executor.cpp. > > Layout of jpackage's native code changed: > - `common`: code shared between all jpackage binaries. > - `applauncher`: launcher only code. > - `applauncherlib`: launcher lib code on Linux and launcher code on other > platforms. > - `applaunchercommon`: code shared between launcher and launcher lib on Linux > and launcher code on other platforms. Alexey Semenyuk has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8254702: jpackage app launcher crashes on CentOS - Changes: - all: https://git.openjdk.java.net/jdk/pull/2320/files - new: https://git.openjdk.java.net/jdk/pull/2320/files/14d277a2..b493bcfd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2320=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2320=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2320.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2320/head:pull/2320 PR: https://git.openjdk.java.net/jdk/pull/2320
