Thanks Alexey for the review! Unless we need another review, would you sponsor the change?
Thanks, -Aleksei On 21/07/2020 21:27, Alexey Semenyuk wrote: > Hi Aleksei, > > Looks good! > > - Alexey > > On 7/21/2020 11:42 AM, Aleksei Voitylov wrote: >> Hi, >> >> This is the updated fix which checks if LD_LIBRARY_PATH has been changed >> during jpackage executions: >> >> http://cr.openjdk.java.net/~avoitylov/webrev.8248239.03/ >> >> The fix stores hash from LD_LIBRARY_PATH in _JPACKAGE_LAUNCHER env >> variable. Until C++14 becomes mandatory for OpenJDK, a custom hash >> algorithm is used because standard C++ hash requires -std=c++11 or >> -std=gnu++11 compiler options. >> >> All test/jdk/tools/jpackage tests but ModulePathTest3.java pass on Linux >> x86_64, MacOSX x86_64, and Windows x86_64. The test ModulePathTest3.java >> is excluded in test/jdk/ProblemList.txt (8248418 generic-all). >> >> Thanks, >> >> -Aleksei >> >> On 26/06/2020 20:23, Alexey Semenyuk wrote: >>> Hi Aleksei, >>> >>> I think the idea of partial reading data from cfg file when the >>> launcher is restarted has a flaw. >>> It would be better if app launcher can detect if it was restarted and >>> if it was, not read cfg file at all, but pass command line arguments >>> as is in JLI_Launch(). >>> Let my think on ideas how to address this. >>> >>> - Alexey >>> >>> On 6/26/2020 7:16 AM, Aleksei Voitylov wrote: >>>> Hi Alexey, >>>> >>>> Thank you for looking into it. As far as using parent pid, it does not >>>> seem to work as example [1] demonstrates. The parent process >>>> remains the >>>> same after re-execution and does not become the current process. >>>> >>>> I checked passing arguments in the "ArgOption" section using several >>>> cases [2, 3, 4] with the proposed fix and app re-execution. The >>>> proposed >>>> fix handles this case well, and the results are the same as without >>>> the >>>> fix when the app is not re-executed. >>>> >>>> Case [3] where only JavaOptions without ArgOptions are passed to app >>>> looks suspicious because default ArgOptions are not used. But it works >>>> the same way without the proposed fix, which seems like a different >>>> issue. According to jpackage documentation: >>>> >>>> --arguments main class arguments >>>> Command line arguments to pass to the main class if no >>>> command >>>> line arguments are given to the launcher. >>>> >>>> I filed a separate issue [5] to handle that. >>>> >>>> Thanks, >>>> -Aleksei >>>> >>>> >>>> [1] >>>> cd jdk-dev >>>> make jdk-image >>>> export PATH=build/linux-x86_64-server-release/images/jdk/bin:$PATH >>>> export >>>> LD_LIBRARY_PATH=build/linux-x86_64-server-release/images/jdk/lib/server >>>> >>>> jpackage --dest output --name app --type app-image --module-path >>>> input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" >>>> --verbose --java-options -Dparam1=Param1 >>>> ./output/app/bin/app -Dparam2=Param2 B B2 B >>>> ------------- >>>> pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app >>>> pid: 15927, parent process: /bin/bash >>>> JvmLauncher args: 10 [/home/sample/jdk-dev/output/app/bin/app >>>> -Dparam1=Param1 --module-path >>>> /home/sample/jdk-dev/output/app/lib/app/mods -m >>>> com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] >>>> pid: 16007, current process: /home/sample/jdk-dev/output/app/bin/app >>>> pid: 15927, parent process: /bin/bash >>>> JvmLauncher args: 15 [/home/sample/jdk-dev/output/app/bin/app >>>> -Dparam1=Param1 --module-path >>>> /home/sample/jdk-dev/output/app/lib/app/mods -m >>>> com.hello/com.hello.Hello -Dparam1=Param1 --module-path >>>> /home/sample/jdk-dev/output/app/lib/app/mods -m >>>> com.hello/com.hello.Hello -Dparam2=Param2 B B2 B ] >>>> ------------- >>>> >>>> [2] >>>> jpackage --dest output --name app --type app-image --module-path >>>> input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" >>>> --java-options -Dparam1=Param1 >>>> ./output/app/bin/app >>>> JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path >>>> output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ] >>>> JvmLauncher args: 9 [output/app/bin/app -Dparam1=Param1 --module-path >>>> output/app/lib/app/mods -m com.hello/com.hello.Hello A A2 A ] >>>> >>>> [3] >>>> jpackage --dest output --name app --type app-image --module-path >>>> input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" >>>> --java-options -Dparam1=Param1 >>>> ./output/app/bin/app -Dparam2=Param2 >>>> JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path >>>> output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ] >>>> JvmLauncher args: 7 [output/app/bin/app -Dparam1=Param1 --module-path >>>> output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 ] >>>> >>>> [4] >>>> jpackage --dest output --name app --type app-image --module-path >>>> input-modules --module com.hello/com.hello.Hello --arguments "A A2 A" >>>> --java-options -Dparam1=Param1 >>>> ./output/app/bin/app -Dparam2=Param2 B B2 B >>>> JvmLauncher args: 10 [output/app/bin/app -Dparam1=Param1 --module-path >>>> output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B >>>> B2 B ] >>>> JvmLauncher args: 10 [output/app/bin/app -Dparam1=Param1 --module-path >>>> output/app/lib/app/mods -m com.hello/com.hello.Hello -Dparam2=Param2 B >>>> B2 B ] >>>> >>>> [5] https://bugs.openjdk.java.net/browse/JDK-8248397 >>>> >>>> >>>> On 24/06/2020 19:34, Alexey Semenyuk wrote: >>>>> Aleksei, >>>>> >>>>> If I get it right, the fix would not work for the case when there are >>>>> `arguments` properties in `ArgOptions` section of a config file. >>>>> Why not just check if the parent process is the same executable as >>>>> the >>>>> current one and if this is the case don't read data from the config >>>>> file but pass executable arguments as is in JLI_Launch() call? >>>>> >>>>> - Alexey >>>>> >>>>> On 6/24/2020 11:48 AM, Aleksei Voitylov wrote: >>>>>> Hi, >>>>>> >>>>>> There are systems that update LD_LIBRARY_PATH or directly return >>>>>> JNI_TRUE in method RequiresSetenv(const char *jvmpath) from >>>>>> java_md.c >>>>>> file to request re-execution of the current executable. This >>>>>> leads to >>>>>> the fact that jpackage application adds some provided arguments >>>>>> twice. >>>>>> >>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8248239 >>>>>> Fix: http://cr.openjdk.java.net/~avoitylov/webrev.8248239.00/ >>>>>> >>>>>> The root cause of the issue is that jpackage application expects one >>>>>> number of arguments but JLI reexecutes them with another number of >>>>>> arguments. >>>>>> For example, a jpackage application can be run with arguments: >>>>>> ./app/bin/app -Dparam2=Param2 B1 B2 B3 >>>>>> it adds arguments from the config file and calls JLI with arguments: >>>>>> app/bin/app -classpath -Dparam1=Param1 -m >>>>>> com.hello/com.hello.Hello >>>>>> -Dparam2=Param2 B1 B2 B3 >>>>>> JLI re-executes the app with new arguments so the app adds some >>>>>> arguments one more time after the re-execution. >>>>>> ./app/bin/app -classpath -Dparam1=Param1 -m >>>>>> com.hello/com.hello.Hello -classpath -Dparam1=Param1 -m >>>>>> com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3 >>>>>> >>>>>> A step by step example that illustrates the issue: >>>>>> >>>>>> Run jpackage to create an executable application: >>>>>> jpackage --dest output --name app --type app-image >>>>>> --module-path >>>>>> input-modules --module com.hello/com.hello.Hello --arguments "A1 A2 >>>>>> A3" >>>>>> --verbose --java-options -Dparam1=Param1 >>>>>> >>>>>> Executable application with the app/lib/app/app.cfg config file is >>>>>> created: >>>>>> ---- app.cfg ---- >>>>>> [Application] >>>>>> app.name=app >>>>>> app.version=1.0 >>>>>> app.runtime=$ROOTDIR/lib/runtime >>>>>> app.identifier=com.hello >>>>>> app.classpath= >>>>>> app.mainmodule=com.hello/com.hello.Hello >>>>>> >>>>>> [JavaOptions] >>>>>> java-options=-Dparam1=Param1 >>>>>> >>>>>> [ArgOptions] >>>>>> arguments=A1 >>>>>> arguments=A2 >>>>>> arguments=A3 >>>>>> ----------- >>>>>> >>>>>> Run the application: >>>>>> ./output/app/bin/app -Dparam2=Param2 B1 B2 B3 >>>>>> >>>>>> Chain of JDK methods execution: >>>>>> >>>>>> LinuxLauncher main() >>>>>> args: 5 [./app/bin/app -Dparam2=Param2 B1 B2 B3 ] >>>>>> AppLauncher createJvmLauncher() >>>>>> args: 4 [-Dparam2=Param2 B1 B2 B3 ] >>>>>> JvmLauncher.cpp Jvm::initFromConfigFile() - adds JavaOptions from >>>>>> app.cfg >>>>>> args: 10 [app/bin/app -classpath -Dparam1=Param1 -m >>>>>> com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3 ] >>>>>> AppLauncher Jvm::launch() - JLI re-executes the app >>>>>> LinuxLauncher main() >>>>>> args: 10 [app/bin/app -classpath -Dparam1=Param1 -m >>>>>> com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3 ] >>>>>> AppLauncher createJvmLauncher() >>>>>> args: 9 [-classpath -Dparam1=Param1 -m >>>>>> com.hello/com.hello.Hello >>>>>> -Dparam2=Param2 B1 B2 B3 ] >>>>>> JvmLauncher.cpp Jvm::initFromConfigFile() - adds JavaOptions from >>>>>> app.cfg >>>>>> args: 15 [./app/bin/app -classpath -Dparam1=Param1 -m >>>>>> com.hello/com.hello.Hello -classpath -Dparam1=Param1 -m >>>>>> com.hello/com.hello.Hello -Dparam2=Param2 B1 B2 B3 ] >>>>>> ^^^ >>>>>> >>>>>> Some arguments like "-classpath -Dparam1=Param1 -m >>>>>> com.hello/com.hello.Hello" are added twice. >>>>>> >>>>>> Tested with test/jdk/tools/jpackage/share/jdk/jpackage with no >>>>>> regressions on Linux, Windows, Mac. On systems affected, the tests >>>>>> now pass. >>>>>> >>>>>> Thanks, >>>>>> >>>>>> -Aleksei >>>>>> >>>>>> >
