Skip to content

SOURCE_DATE_EPOCH build arg injection fixes #1675

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.

Sign up for GitHub

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

Merged
tonistiigi merged 2 commits into from
Mar 22, 2023
Merged

SOURCE_DATE_EPOCH build arg injection fixes #1675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d4223e
build: move SOURCE_DATE_EPOCH parsing into option generation
jedevc Mar 14, 2023
7805314
bake: move SOURCE_DATE_EPOCH parsing to overrides
jedevc Mar 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions bake/bake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,19 +140,6 @@ func ReadTargets(ctx context.Context, files []File, targets, overrides []string,
}
}

// Propagate SOURCE_DATE_EPOCH from the client env.
// The logic is purposely duplicated from `build/build`.go for keeping this visible in `bake --print`.
if v := os.Getenv("SOURCE_DATE_EPOCH"); v != "" {
for _, f := range m {
if f.Args == nil {
f.Args = make(map[string]*string)
}
if _, ok := f.Args["SOURCE_DATE_EPOCH"]; !ok {
f.Args["SOURCE_DATE_EPOCH"] = &v
}
}
}

return m, n, nil
}

Expand Down
7 changes: 0 additions & 7 deletions build/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,13 +604,6 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt Op
}
}

// Propagate SOURCE_DATE_EPOCH from the client env
if v := os.Getenv("SOURCE_DATE_EPOCH"); v != "" {
if _, ok := so.FrontendAttrs["build-arg:SOURCE_DATE_EPOCH"]; !ok {
so.FrontendAttrs["build-arg:SOURCE_DATE_EPOCH"] = v
}
}
Comment on lines -607 to -612
Copy link
Copy Markdown
Member

@crazy-max crazy-max Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the case where buildx is used as lib (compose), it is not going to work anymore I guess?

Copy link
Copy Markdown
Collaborator Author

@jedevc jedevc Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm you're right. I'm not quite sure if that's a bug or a feature though 🤔

I'm leaning towards it's probably best that when used as a library buildx should avoid reading from the environment as much as possible. We also have some minor precedent here - DOCKER_DEFAULT_PLATFROM.

Though I wonder about cases like BUILDX_NO_DEFAULT_ATTESTATIONS. On the one hand, it makes sense to propagate them, on the other, it could be confusing to the library consumer if the environment overrides the options that they have passed to Buildx.

IMO, in the future we should push all environment variables that affect the build solve request (so not including buildx driver env vars, etc) into the command layer, and then we should provide a utility method to parse environment variables and modify the build options from them:

buildx/controller/pb/controller.proto

Lines 47 to 73 in 4a73abf

message BuildOptions {
string ContextPath = 1;
string DockerfileName = 2;
string PrintFunc = 3;
map<string, string> NamedContexts = 4;
repeated string Allow = 5;
repeated Attest Attests = 6;
map<string, string> BuildArgs = 7;
repeated CacheOptionsEntry CacheFrom = 8;
repeated CacheOptionsEntry CacheTo = 9;
string CgroupParent = 10;
repeated ExportEntry Exports = 11;
repeated string ExtraHosts = 12;
map<string, string> Labels = 13;
string NetworkMode = 14;
repeated string NoCacheFilter = 15;
repeated string Platforms = 16;
repeated Secret Secrets = 17;
int64 ShmSize = 18;
repeated SSH SSH = 19;
repeated string Tags = 20;
string Target = 21;
UlimitOpt Ulimits = 22;
CommonOptions Opts = 24;
}

Does that seem somewhat reasonable? If so, I'll add a TODO in the code - it's tricky to handle right now, since I think with the controller code, I'd expect we'd try and get library consumers of Buildx to use that API instead of using build.Build?

Copy link
Copy Markdown
Member

@crazy-max crazy-max Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM 👍

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

./cc FYI @glours @ndeloof (for compose)


// set platforms
if len(opt.Platforms) != 0 {
pp := make([]string, len(opt.Platforms))
Expand Down
13 changes: 13 additions & 0 deletions commands/bake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,19 @@ func runBake(dockerCli command.Cli, targets []string, in bakeOptions, cFlags com
return err
}

if v := os.Getenv("SOURCE_DATE_EPOCH"); v != "" {
// TODO: extract env var parsing to a method easily usable by library consumers
for _, t := range tgts {
if _, ok := t.Args["SOURCE_DATE_EPOCH"]; ok {
continue
}
if t.Args == nil {
t.Args = map[string]*string{}
}
t.Args["SOURCE_DATE_EPOCH"] = &v
}
}

// this function can update target context string from the input so call before printOnly check
bo, err := bake.TargetsToBuildOpt(tgts, inp)
if err != nil {
Expand Down
7 changes: 7 additions & 0 deletions commands/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,13 @@ func (o *buildOptions) toControllerOptions() (controllerapi.BuildOptions, error)
Opts: &o.CommonOptions,
}

// TODO: extract env var parsing to a method easily usable by library consumers
if v := os.Getenv("SOURCE_DATE_EPOCH"); v != "" {
if _, ok := opts.BuildArgs["SOURCE_DATE_EPOCH"]; !ok {
opts.BuildArgs["SOURCE_DATE_EPOCH"] = v
}
}

inAttests := append([]string{}, o.attests...)
if o.provenance != "" {
inAttests = append(inAttests, buildflags.CanonicalizeAttest("provenance", o.provenance))
Expand Down