On Fri, 29 Jan 2021 23:06:20 GMT, Alexey Semenyuk <asemen...@openjdk.org> 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
