Add support for Go build flags #340

HeavyWombat · 2021-04-19T07:44:39Z

Fixes #316

There are use cases, where multiple Go build flags need to be set. However, the environment variable to pass flags to Go build has some limits for ldFlags.

Proposed changes:

Add GoReleaser inspired configuration section to .ko.yaml to support setting specific Go build and ldFlags to be used by the build.
Add support for environment variables KO_GOBUILD_FLAGS to add extra build flags and KO_GOBUILD_LDFLAGS to add extra ldFlags.

google-cla · 2021-04-19T07:44:43Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

HeavyWombat · 2021-04-19T07:46:02Z

@googlebot I signed it!

jonjohnsonjr

Thanks for doing this -- a few comments, but it's looking pretty good.

pkg/build/options.go

README.md

pkg/build/gobuild.go

jonjohnsonjr · 2021-04-30T21:09:42Z

pkg/build/options.go

@@ -45,6 +46,15 @@ func WithDisabledOptimizations() Option {
 	}
 }

+// WithGoReleaserBuildConfig is a functional option for providing build
+// settings based on a GoReleaser style configuration.
+func WithGoReleaserBuildConfig(buildCfgOverrides map[string]config.Build) Option {


I think this is impossible for external dependents to use because config.Build is internal/. (cc @halvards)

We may want to have a pkg/config (or something) that exposes the usable subset of goreleaser's config.Build, then do some translation to talk advantage of their stuff.

Good point. Thanks. I moved it to pkg/config. However, I am wondering whether maybe something like pkg/ref/goreleaser/config would be better, because pkg/config is pretty generic.

@jonjohnsonjr Did you have to think about the name of package?

Sorry, missed this comment. That's a good point.

My intuition is that this would only affect build config, so it might make more sense to flip the name and call this build.Config instead of config.Build?

I'm indifferent re: config vs configuration, but this seems reasonable enough:

builder.Build(..., build.WithConfig(build.Config{}))

We already have build.Result so it fits in well. If we want to have a separate configuration for e.g. the publisher stuff (currently set by KO_DOCKER_REPO), that could be mirrored as publisher.Config. I think scoping the configuration to the relevant package is probably best, but I'd be interested in other ideas.

Sorry for the back and forth on this :P

No problem, not at all. I am here to learn. Naming things can be hard and build.Config makes perfect sense while also improves the readability of the code. I will change that right away.

jonjohnsonjr · 2021-05-18T17:35:03Z

pkg/build/config.go

+import "strings"
+
+// Note: The structs, types, and functions are based upon GoReleaser build
+// configuration to have a loosly compatible YAML configuration:


Suggested change

// configuration to have a loosly compatible YAML configuration:

// configuration to have a loosely compatible YAML configuration:

jonjohnsonjr · 2021-05-18T18:02:12Z

pkg/build/options.go

@@ -45,6 +45,15 @@ func WithDisabledOptimizations() Option {
 	}
 }

+// WithConfig is a functional option for providing build settings based on a
+// GoReleaser Build influenced configuration.
+func WithConfig(buildCfgOverrides map[string]Config) Option {


Document what the key is here.

jonjohnsonjr · 2021-05-18T18:05:03Z

pkg/build/config.go

+
+// Config contains the build configuration section. The name was changed from
+// the original GoReleaser name to match better with the ko namings.
+type Config struct {


I would comment out anything that we don't currently respect and put a big fat TODO comment at the top, maybe pointing to an existing issue or a new issue to track progress.

I believe we only respect Ldflags and Flags currently?

Yes, it is only Ldflags and Flags with this PR. In the project I am currently working on, I see the potential that maybe something like ModTimestamp and Env would be interesting, too. I would like to tackle that with subsequent PRs.

jonjohnsonjr · 2021-05-18T18:05:38Z

pkg/build/config.go

+	Goarm        []string    `yaml:",omitempty"`
+	Gomips       []string    `yaml:",omitempty"`
+	Targets      []string    `yaml:",omitempty"`
+	Dir          string      `yaml:",omitempty"`


@halvards we might be able to use Dir from this instead of #365

jonjohnsonjr · 2021-05-18T20:22:30Z

I think we'll want to combine the changes from #365 with this PR.

This PR uses build.Config.Dir with build.Config.Main to qualify local import paths, but we should probably also do what #365 is doing and use Dir as the working directory for the go build. I'm also not sure how this interacts with kodata -- we might want to check on that.

HeavyWombat · 2021-05-19T20:51:10Z

I think we'll want to combine the changes from #365 with this PR.

This PR uses build.Config.Dir with build.Config.Main to qualify local import paths, but we should probably also do what #365 is doing and use Dir as the working directory for the go build. I'm also not sure how this interacts with kodata -- we might want to check on that.

Changing the code to make sure that go build uses the directory specified in build.Config.Dir as the working directory required a bit of a rewrite of some signatures and the flow. I pushed the idea I have for that and this also placed the kodata directory under /var/run/ko. However, it only does that for the go build. In #365, the working directory change is also done for go list and go env. Especially, for go list I am not sure if changing the working directory is desired in all cases, or at least I am not sure how it will work if the modules file is at the project root directory while the respective working directory would be a sub-directory. If we want to have the working directory fix for all go commands, I was wondering whether it would be easier to introduce the working directory as a Value into the Context that is available in all functions already rather than adding a directory string to all function signatures.

jonjohnsonjr · 2021-06-09T15:48:16Z

Apologies for taking a while to get around to looking at this. It's been a while since I got too deep into the ko codebase (kodebase?), so it's difficult to reason about everything affected by this change. I'm having a hard time really understanding the semantics of this config file... would you mind adding a comment to the Main and Dir fields describing exactly what they control? It will help me juggle this and #365 better, I think.

Especially, for go list I am not sure if changing the working directory is desired in all cases, or at least I am not sure how it will work if the modules file is at the project root directory while the respective working directory would be a sub-directory.

Interesting. This has been challenging in the past when dealing with go submodules within a repository.

I was wondering whether it would be easier to introduce the working directory as a Value into the Context that is available in all functions already rather than adding a directory string to all function signatures.

I think I prefer the simplicity of changing the function signatures. It makes it easier to review at least.

HeavyWombat · 2021-06-21T14:15:58Z

Apologies for taking a while to get around to looking at this. It's been a while since I got too deep into the ko codebase (kodebase?), so it's difficult to reason about everything affected by this change. I'm having a hard time really understanding the semantics of this config file... would you mind adding a comment to the Main and Dir fields describing exactly what they control? It will help me juggle this and #365 better, I think.

Especially, for go list I am not sure if changing the working directory is desired in all cases, or at least I am not sure how it will work if the modules file is at the project root directory while the respective working directory would be a sub-directory.

Interesting. This has been challenging in the past when dealing with go submodules within a repository.

I was wondering whether it would be easier to introduce the working directory as a Value into the Context that is available in all functions already rather than adding a directory string to all function signatures.

I think I prefer the simplicity of changing the function signatures. It makes it easier to review at least.

@jonjohnsonjr I added comments to Dir and Main the way I understand them. This way it is more clear when looking at the code without requiring any context of GoReleaser. Also, I changed the config code to be able to properly test the permutations of Dir and Main. However, I do think that most users of this feature will only use Main.

There are use cases, where multiple Go build flags need to be set. However, the environment variable to pass flags to Go build has some limits for `ldFlags`. Add GoReleaser inspired configuration section to `.ko.yaml` to support setting specific Go build and ldFlags to be used by the build. Like GoReleaser the content of the configuration can use Go templates. Currently, only a section for environment variables is included. In order to reduce dependency overhead, only the respective config structs from https://github.com/goreleaser/goreleaser/blob/master/pkg/config/config.go are used internally to load from `.ko.yaml`.

jonjohnsonjr · 2021-06-21T20:45:28Z

@imjasonh want to take a look? I think we're getting close to something that's actually not bad!

This adds the `Build()` method for building artifacts using ko. It supports both publishing the resulting image to a registry, and sideloading it to the local Docker daemon. The `temporary.go` file contains the structs intended to be added to the schema. This implementation is still missing the following features: - integration test - dependencies (for file watching) - insecure registries - debug mode - support for `go` flags and environment variables (waiting on ko-build/ko#340) - actually plumbing the builder into the Skaffold CLI and API :-) Tracking: GoogleContainerTools#6041

imjasonh

Looks great! Thanks for your hard work on this @HeavyWombat

@jonjohnsonjr Anything else you want to see before we merge this? We should probably cut a release after so people can use it more easily.

jonjohnsonjr · 2021-07-02T15:40:49Z

I want to experiment for a bit before cutting a release, I think.

HeavyWombat · 2021-07-05T18:52:45Z

I want to experiment for a bit before cutting a release, I think.

Thanks for looking into it. If there is anything I can help with, please let me know.

This adds the `Build()` method for building artifacts using ko. It supports both publishing the resulting image to a registry, and sideloading it to the local Docker daemon. The Skaffold docker client is used in the ko builder. This ensures that any Minikube config set by Skaffold is used. The ko builder uses the docker client when sideloading images to the docker daemon. The `temporary.go` file contains the structs intended to be added to the schema. The additions to the design proposal explain image naming for the ko builder, specifically how container images are named and how Go import paths are resolved when using the proposed ko builder. This commit includes ko builder unit tests for non-current-working-directory workspace dirs. These verify that the ko builder works even if the context specified in `skaffold.yaml` differs from the current working directory. This implementation is still missing the following features: - integration test - dependencies (for file watching) - insecure registries - debug mode - support for `go` flags and environment variables (based on ko-build/ko#340) - actually plumbing the builder into the Skaffold CLI and API :-) Tracking: GoogleContainerTools#6041

* Add core ko builder implementation This adds the `Build()` method for building artifacts using ko. It supports both publishing the resulting image to a registry, and sideloading it to the local Docker daemon. The Skaffold docker client is used in the ko builder. This ensures that any Minikube config set by Skaffold is used. The ko builder uses the docker client when sideloading images to the docker daemon. The `temporary.go` file contains the structs intended to be added to the schema. The additions to the design proposal explain image naming for the ko builder, specifically how container images are named and how Go import paths are resolved when using the proposed ko builder. This commit includes ko builder unit tests for non-current-working-directory workspace dirs. These verify that the ko builder works even if the context specified in `skaffold.yaml` differs from the current working directory. This implementation is still missing the following features: - integration test - dependencies (for file watching) - insecure registries - debug mode - support for `go` flags and environment variables (based on ko-build/ko#340) - actually plumbing the builder into the Skaffold CLI and API :-) Tracking: #6041 * Improve ko Builder unit tests Split out the confusing end-to-end unit test with tests for individual functions. * Reorder test inputs and expected outputs Also fix typo in design proposal. * Add TODO to update import path Reminders to update the import path once the contents of `pkg/skaffold/build/ko/schema` have been added to the real schema in `pkg/skaffold/schema/latest/v1`. * Fix lint errors for TODO comments in import block Adding the TODO in the import block resulted in gci linter errors: https://app.travis-ci.com/github/GoogleContainerTools/skaffold/jobs/530642848#L313

Matches the GoReleaser format. Related: ko-build#340

Enables programmatically overriding build configs when ko is embedded in another tool. Related: ko-build#340, ko-build#419

Matches the GoReleaser format. Related: ko-build#340

Enables programmatically overriding build configs when ko is embedded in another tool. Related: ko-build#340, ko-build#419

* Enable setting environment variables in .ko.yaml Matches the GoReleaser format. Related: #340 * Use different env example

Enables programmatically overriding build configs when ko is embedded in another tool. Related: ko-build#340, ko-build#419

* Set build config via BuildOptions Enables programmatically overriding build configs when ko is embedded in another tool. Related: #340, #419 * Use local registry for base images in unit tests Tests create a local registry (using ggcr) with a dummy base image. This speeds up tests, since they don't need to hit gcr.io to fetch the default distroless base image. * Update function comment to refer to random image

google-cla bot added the cla: no label Apr 19, 2021

google-cla bot added cla: yes and removed cla: no labels Apr 19, 2021

HeavyWombat force-pushed the add/go-build-flag-overrides branch 2 times, most recently from 7d6437a to 920d8cb Compare April 19, 2021 08:22

jonjohnsonjr reviewed Apr 20, 2021

View reviewed changes

pkg/build/options.go Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

pkg/build/gobuild.go Outdated Show resolved Hide resolved

HeavyWombat force-pushed the add/go-build-flag-overrides branch 2 times, most recently from 037a944 to a88c9be Compare April 30, 2021 20:56

jonjohnsonjr reviewed Apr 30, 2021

View reviewed changes

HeavyWombat force-pushed the add/go-build-flag-overrides branch from a88c9be to 9b1e390 Compare May 1, 2021 08:40

HeavyWombat force-pushed the add/go-build-flag-overrides branch from 9b1e390 to 0fbec7b Compare May 18, 2021 15:57

jonjohnsonjr reviewed May 18, 2021

View reviewed changes

HeavyWombat force-pushed the add/go-build-flag-overrides branch from 0fbec7b to 04135ed Compare May 18, 2021 19:44

HeavyWombat requested a review from jonjohnsonjr May 18, 2021 19:45

HeavyWombat force-pushed the add/go-build-flag-overrides branch 2 times, most recently from 270915c to ead5e7f Compare May 19, 2021 20:42

halvards mentioned this pull request Jun 18, 2021

Add ko builder GoogleContainerTools/skaffold#6041

Closed

HeavyWombat force-pushed the add/go-build-flag-overrides branch from ead5e7f to 03e4b3e Compare June 21, 2021 14:11

HeavyWombat force-pushed the add/go-build-flag-overrides branch from 03e4b3e to 606e66e Compare June 21, 2021 14:17

halvards mentioned this pull request Jun 22, 2021

Add core ko builder implementation GoogleContainerTools/skaffold#6054

Merged

imjasonh approved these changes Jul 2, 2021

View reviewed changes

jonjohnsonjr merged commit ab4d264 into ko-build:main Jul 2, 2021

HeavyWombat deleted the add/go-build-flag-overrides branch July 5, 2021 18:52

halvards added a commit to halvards/ko that referenced this pull request Aug 18, 2021

Enable setting environment variables in .ko.yaml

799f6fc

Matches the GoReleaser format. Related: ko-build#340

halvards mentioned this pull request Aug 18, 2021

Enable setting environment variables in .ko.yaml #419

Merged

halvards added a commit to halvards/ko that referenced this pull request Aug 19, 2021

Set build config via BuildOptions

9bae8e0

Enables programmatically overriding build configs when ko is embedded in another tool. Related: ko-build#340, ko-build#419

halvards mentioned this pull request Aug 19, 2021

Set build config via BuildOptions #421

Merged

halvards added a commit to halvards/ko that referenced this pull request Aug 22, 2021

Enable setting environment variables in .ko.yaml

e865741

Matches the GoReleaser format. Related: ko-build#340

halvards added a commit to halvards/ko that referenced this pull request Aug 22, 2021

Set build config via BuildOptions

31f812c

Enables programmatically overriding build configs when ko is embedded in another tool. Related: ko-build#340, ko-build#419

jonjohnsonjr pushed a commit that referenced this pull request Aug 23, 2021

Enable setting environment variables in .ko.yaml (#419)

9a256a4

* Enable setting environment variables in .ko.yaml Matches the GoReleaser format. Related: #340 * Use different env example

halvards added a commit to halvards/ko that referenced this pull request Aug 25, 2021

Set build config via BuildOptions

bfd6cee

Enables programmatically overriding build configs when ko is embedded in another tool. Related: ko-build#340, ko-build#419

jonjohnsonjr mentioned this pull request Aug 31, 2021

Build config assumes app sources are in current working directory #422

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Go build flags #340

Add support for Go build flags #340

HeavyWombat commented Apr 19, 2021

google-cla bot commented Apr 19, 2021

HeavyWombat commented Apr 19, 2021

jonjohnsonjr left a comment

jonjohnsonjr Apr 30, 2021

HeavyWombat May 1, 2021

HeavyWombat May 12, 2021

jonjohnsonjr May 12, 2021

HeavyWombat May 18, 2021

jonjohnsonjr May 18, 2021

jonjohnsonjr May 18, 2021

jonjohnsonjr May 18, 2021

HeavyWombat May 18, 2021

jonjohnsonjr May 18, 2021

jonjohnsonjr commented May 18, 2021

HeavyWombat commented May 19, 2021

jonjohnsonjr commented Jun 9, 2021

HeavyWombat commented Jun 21, 2021

jonjohnsonjr commented Jun 21, 2021

imjasonh left a comment

jonjohnsonjr commented Jul 2, 2021

HeavyWombat commented Jul 5, 2021

	// configuration to have a loosly compatible YAML configuration:
	// configuration to have a loosely compatible YAML configuration:

Add support for Go build flags #340

Add support for Go build flags #340

Conversation

HeavyWombat commented Apr 19, 2021

google-cla bot commented Apr 19, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

HeavyWombat commented Apr 19, 2021

jonjohnsonjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonjohnsonjr commented May 18, 2021

HeavyWombat commented May 19, 2021

jonjohnsonjr commented Jun 9, 2021

HeavyWombat commented Jun 21, 2021

jonjohnsonjr commented Jun 21, 2021

imjasonh left a comment

Choose a reason for hiding this comment

jonjohnsonjr commented Jul 2, 2021

HeavyWombat commented Jul 5, 2021