jboss-container-images / openjdk

Source To Image (S2I) image for Red Hat OpenShift providing OpenJDK
Apache License 2.0
53 stars 58 forks source link

[OPENJDK-2850] assemble: binary: Don't set times on directories #470

Closed jmtd closed 2 months ago

jmtd commented 3 months ago

If the S2I build (in --as-dockerfile mode, for the standalone s2i, or by default inside OpenShift) runs as a user other than 185 (such as s2i's default of 1001), attempting to set the timestamp of /deployments will fail, causing the build to fail.

https://issues.redhat.com/browse/OPENJDK-2850

See also https://issues.redhat.com/browse/OPENJDK-2408

jmtd commented 3 months ago

Test " Scenario: Ensure mtime is preserved for build artifacts (OPENJDK-2408)" passes

jmtd commented 3 months ago

New test passes locally (test suite on GHA is presently broken again :()

jmtd commented 2 months ago

It seems convoluted, TBH. The fix for OPENJDK-2408 would only need rsync -rlt (over --archive). I take it that would have the same problem?

Afraid so; -t is the issue (although other stuff that --archive tries to do would likely also fail, such as setting xattrs).

rsync has special behaviour when you suffix a / to a source path, which was a promising-looking fix:

A trailing slash on the source changes this behavior to avoid creating an additional directory level at the destination. You can think of a trailing / on a source as meaning "copy the contents of this directory" as opposed to "copy the directory by name"

unfortunately it goes on to say

but in both cases the attributes of the containing directory are transferred to the containing directory on the destination.

The globstar is needed to match any hidden files in the source directory. I ran out of time to write an explicit test for that. The alternative $srcdir/* $srcdir/.* would match . and ... The subshell (to avoid setting/altering globstar in the parent context) is just an abundance of caution, and might not strictly be necessary, but I didn't want to spend the time analysing the call chain to be certain.

Anyway, it seems OK if it fixes the problem.

I think it does that, at least!

Thanks for the 👍🏼