exporter/containerimage: new option: rewrite-timestamp (Apply SOURCE_DATE_EPOCH to file timestamps)#4057
Conversation
b32dffa to
cabb867
Compare
SOURCE_DATE_EPOCH to file timestamps)
cabb867 to
de78107
Compare
15e643d to
f54de34
Compare
|
@tonistiigi ? |
f54de34 to
374410d
Compare
|
@AkihiroSuda I think until we figure out how this will work with |
374410d to
39c3a49
Compare
I guess the entire logic should be just decoupled from (So, the epoch will be applied to the base layers as well) |
39c3a49 to
01c3d22
Compare
|
Updated as above. The |
46f3787 to
4963e73
Compare
4963e73 to
a3847fb
Compare
|
@felixdesouza Thanks, fixed in the latest commit. (the tar writer was not flushed) |
|
Ah, beat me to it! I think there's one last flush missing, at the end of the archive, we'll get an |
fdde4db to
1ed7a11
Compare
|
Should be fixed in the latest commit |
1ed7a11 to
52fb34e
Compare
|
Great to see this being worked on, thanks! I wonder one thing: would it be beneficial to also add test cases that'd check if the digest does not change? Having reproducible images is super great but I'm worried some other contributor will break it by accident in the future. |
|
|
||
| // Resolve converters | ||
| layerConvertFunc, err := getConverter(ctx, ref.cm.ContentStore, desc, comp) | ||
| layerConvertFunc, err := converter.New(ctx, ref.cm.ContentStore, desc, comp) |
There was a problem hiding this comment.
can be follow-up: This New pattern doesn't work with function anymore and should probably return a struct/interface.
There was a problem hiding this comment.
New implies a new instance of a struct.
There was a problem hiding this comment.
Newimplies a new instance of a struct.
I'm not sure.
I thought it can return any.
exporter/containerimage/export.go
Outdated
| // e.unpackImage cannot be used because src ref does not point to the rewritten image | ||
| err = errors.New("exporter option \"rewrite-epoch\" conflicts with \"unpack\"") | ||
| } else { | ||
| err = e.unpackImage(ctx, img, src, session.NewGroup(sessionID)) |
There was a problem hiding this comment.
Should we change this function so that it takes Result[Remote] as parameter. This can be follow-up but I do think we should implement it and not error. Same for push.
There was a problem hiding this comment.
Sorry, not sure how it relates to this PR
There was a problem hiding this comment.
There are new conditions now for unpack and push, instead of reusing the code-path. And unpack case errors.
There was a problem hiding this comment.
Got it, but let me defer that to a follow-up 🙏
52fb34e to
852e57b
Compare
852e57b to
8579128
Compare
8579128 to
e979a33
Compare
tonistiigi
left a comment
There was a problem hiding this comment.
More follow-ups: when the layer conversion happens, we should show a progress line about it to the user.
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
e979a33 to
0265bb7
Compare
done |
|
BTW, on the second thought,
|
0265bb7 to
8fcc411
Compare
|
Changed to (Backup: commit 0265bb7 ( |
SOURCE_DATE_EPOCH to file timestamps)SOURCE_DATE_EPOCH to file timestamps)
6a8e2e3 to
1b59dfc
Compare
rewrite-timestamp rewrites timestamps in layers for reproducible
builds.
When a layer is rewritten, an annotation "buildkit/rewritten-timestamp=<INT64>"
is set to the layer.
Example:
```
buildctl build
--frontend dockerfile.v0 \
--opt build-arg:SOURCE_DATE_EPOCH=${SOURCE_DATE_EPOCH} \
--output type=oci,dest=dest.tar,rewrite-timestamp=true \
...
```
Alternative to PR 3560
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
1b59dfc to
2f8292c
Compare


rewrite-timestamprewrites timestamps in layers for reproducible builds.When a layer is rewritten, an annotation
buildkit/rewritten-timestamp=<INT64>is set to the layer.Example:
Alternative to:
With this commit, the following workarounds mentioned in
docs/build-repro.mdare no longer needed for reproducible builds:Backup
39c3a49 : Abandoned version, contaminates cache