-
Notifications
You must be signed in to change notification settings - Fork 61
[OPENJDK-3646] clean up jdeps *.txt files unless S2I_DELETE_SOURCE is set #566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
This looks good: I'd like to see a Behave test to check the behaviour. |
S2I_JLINK_TEMP_PATH is defined in modules/jlink, and is referenced by scripts in that module as well as modules/s2i/bash/artifacts/usr/local/s2i/assemble. At the moment, modules/s2i/bash requires the jlink module, so S2I_JLINK_TEMP_PATH is guaranteed to be defined, but, separately I was wondering whether that dependency should be removed (moved to the image descriptor for the TP image, i.e., ubi9-openjdk-21 at this point). It doesn't matter so long as we build the tech-preview image from the jlink-dev branch and don't build the other product images from that branch, but a consequence is if you did build the other product images frmo this branch, all the builder images would pick up the jlink module. That would be a blocker for a future merge down to |
modules/jlink/artifacts/opt/jboss/container/java/jlink/mkstrippeddeps.sh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per inline comments
FWIW I think it would be easiest to extend the existing jlink tests that were fixed in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Given s2i build https://github.com/rh-openjdk/openjdk-container-test-applications from quarkus-quickstarts/getting-started-3.9.2-uberjar | ||
| variable | value | | ||
| S2I_ENABLE_JLINK | true | | ||
| S2I_DELETE_SOURCE | true | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value of S2I_DELETE_SOURCE is true. What we really need to test is that, when set to false, we don't clean them up. I guess testing both scenarios is worthwhile.
Addresses https://issues.redhat.com/browse/OPENJDK-3646
Creates all of the temporary files used by the jlink workflow (deps.txt, etc) under /tmp/jlink, this can be controlled by the S2I_JLINK_TEMP_PATH variable. Once the jlink workflow completes it then checks S2I_DELETE_SOURCE and removes /tmp/jlink if set.
To test: