JetBrains / JetBrainsRuntime

Runtime environment based on OpenJDK for running IntelliJ Platform-based products on Windows, macOS, and Linux
GNU General Public License v2.0
1.32k stars 196 forks source link

JBR-6246 Do not overwrite CDS archives after created with jlink #303

Closed mkartashev closed 8 months ago

mkartashev commented 8 months ago

The CDS files (*.jsa) are created with the option --generate-cds-archive of jlink. The modules file at their creation time must be the same at run time; so when they are overwritten with the files from $IMAGES_DIR where modules is different, we get a warning at run time when those CDS files are used. The fix is to not overwrite *.jsa files after they were created.

vprovodin commented 8 months ago

The test test/jdk/jb/build/CDSArchivesTest.sh currently checks that jsa-files were included into jbrsdk-distributions, but after these changes jsa-files will be included into both distributions jbr and jbrsdk. The test should be modified to check both distributions.

vprovodin commented 8 months ago

windows-aarch64 do not have jsa-files in spite of the option --generate-cds-archive. Note: for windows these files should be in bin/server (not in lib/server) I suspect the issue with windows-aarch64 is originated from Microsoft - I was going to submit this issue to JBS

mkartashev commented 8 months ago

...after these changes jsa-files will be included into both distributions jbr and jbrsdk. The test should be modified to check both distributions.

I'm not sure this is the case; if they are included into jbr also, I would prefer to remove them rather than to modify the test. As per our earlier agreement, CDS archives only make sense for jbrsdk.

mkartashev commented 8 months ago

This commit does not add CDS to jbr bundles and does not create CDS files on Windows/aarch64. Please, take a look: https://github.com/JetBrains/JetBrainsRuntime/pull/303/commits/bc239df27eac50caf5d5d347d34084821e4e5ef6

vprovodin commented 8 months ago

IMPLEMENTOR_VERSION is incorrectly defined in Linux so the test wrongly fails

mkartashev commented 8 months ago

Merged