ko-build / ko

Build and deploy Go applications
https://ko.build
Apache License 2.0
7.6k stars 398 forks source link

Consider not adding `-trimpath` flag by default when disabling compiler optimizations #500

Closed halvards closed 2 years ago

halvards commented 2 years ago

ko adds the -trimpath flag if no build config is specified: https://github.com/google/ko/blob/5617d1ebf8560df778aa884fa328fe05f62bdb5e/pkg/build/gobuild.go#L562

This removes information necessary for remote debugging, see: https://github.com/GoogleContainerTools/skaffold/issues/6843#issuecomment-966758261

Users who want to debug would typically also use the --disable-optimizations flag.

Should we default to not add -trimpath if disableOptimizations is true?

In other words, change this conditional from !ok to !ok && !g.disableOptimizations?

A workaround is to ask users to specify a build config if they want to control whether the -trimpath flag is added.

halvards commented 2 years ago

(By the way, I'd be happy to implement it.)

imjasonh commented 2 years ago

Not making a judgement either way for now, but this issue explains why we added -trimpath in the first place: https://github.com/google/ko/issues/71

halvards commented 2 years ago

Thanks for the context, I :heart: reproducible builds too.

But when interactively debugging it's not as important IMHO. Or is --disable-optimizations used for other purposes too?

imjasonh commented 2 years ago

I don't know enough about Skaffold, but does it have some "run this in debug mode" option that could disable -trimpath for ko builds? Or is it always "run this code; I may want to debug it later"?

If Skaffold's use case is iterative (local?) development and not releasing to prod, I care a lot less about the reproducibility of Skaffold's ko builds, you could maybe even just have Skaffold always unset -trimpath and get everything you want. I'm not sure this even needs changes in ko to enable that.

halvards commented 2 years ago

Thanks for the reply, I'm definitely happy to discuss other solutions!

To your questions:

does it have some "run this in debug mode"

Yes, that's exactly the use case. Users run skaffold debug and attach a debugger, or perform an equivalent action using the Cloud Code IDE extensions. Reproducibility is not important for this action.

you could maybe even just have Skaffold always unset -trimpath

(warning: long answer!)

The integration between Skaffold and ko includes goals of:

  1. Making it easy for existing Skaffold users to adopt ko - e.g., just change docker to ko in your Skaffold config and you're good to go (for simple scenarios); and
  2. Making it easy for existing ko users to adopt Skaffold. Users who have a .ko.yaml file specifying base image overrides and/or build configs do not have to duplicate that configuration in Skaffold. Instead, they can create a minimal Skaffold config file saying they want to use ko (literally ko: {}), and the build picks up the configuration from .ko.yaml.

The integration achieves the second point by programmatically setting the ko BaseImage and BuildConfigs only if the Skaffold config includes those fields. If they're unset, ko reads from .ko.yaml instead: https://github.com/google/ko/blob/5617d1ebf8560df778aa884fa328fe05f62bdb5e/pkg/commands/options/build.go#L100 https://github.com/google/ko/blob/5617d1ebf8560df778aa884fa328fe05f62bdb5e/pkg/commands/options/build.go#L120

The challenge right now is that the only way to prevent ko from adding -trimpath is to have a build config for the image being built: https://github.com/google/ko/blob/5617d1ebf8560df778aa884fa328fe05f62bdb5e/pkg/build/gobuild.go#L559-L563

Users who don't specify a build config in either .ko.yaml or skaffold.yaml will find it challenging to interactively debug their images. We could instruct users to just create a build config in either file to control flags, however users may want it for debug but not for other actions, and they'd need a way to toggle the behavior.

As you suggest, we could always pass a build config to ko when debugging, regardless of whether the user has specified a build config or not. However, if we do this, then we don't get the behavior of falling back to build configs in .ko.yaml. In other words, we'd have to tell users that "when you're debugging, build configuration in .ko.yaml isn't used".

An alternative solution is to add a NoTrimpath BuildOption to ko. Tools like Skaffold could set this when running in debug mode. This would work well. Other tools that want to implement similar behavior would have to ensure they set both NoTrimpath and DisableOptimizations when users want to debug.

Thanks for reading all this! I hope it makes sense, and I'd be happy to clarify.

jonjohnsonjr commented 2 years ago

If -trimpath makes it harder to debug things, and --disable-optimizations is primarily for debugging, it seems pretty reasonable to me to change this behavior...

We could also maybe introduce a --debug flag to better express the intent?

halvards commented 2 years ago

Do you mean rename the existing --disable-optimizations flag to --debug (and keeping the old one as a hidden flag), or do you mean two flags?

If we go with two flags, --debug is a reasonable name. But if we only alter the behavior of the existing --disable-optimizations flag, I'd be happy to keep the name as is.

We're using distroless as the default base image, and this flag doesn't change the base image to a debug image. Do we think that users would expect this? In other words, could users interpret --debug as "add a shell so I can poke around" rather than "I want to attach a debugger"? (I'm not suggesting that we change base image behavior.)

imjasonh commented 2 years ago

+1 to separate flags, and maybe considering deprecating/hiding --disable-optimizations eventually in favor of --debug. I don't consider reproducibility an "optimization", so I don't think I'd like to have -trimpath affected by that path.

We're using distroless as the default base image, and this flag doesn't change the base image to a debug image. Do we think that users would expect this? In other words, could users interpret --debug as "add a shell so I can poke around" rather than "I want to attach a debugger"? (I'm not suggesting that we change base image behavior.)

I think we should just solve this with clear documentation about what --debug does, even just your points above verbatim in the README.

Thanks for driving this and for being so through and considerate of all the options and user expectations. 👍

jonjohnsonjr commented 2 years ago

Skaffold doesn't use these flags, right? I'd be ok with exposing the -trimpath functionality as a build.Option.

I don't know if we want to drop DisableOptimizations and just lump this and trimpath into a single option that appends to config.Flags or just add a TrimPath option that defaults to true...

halvards commented 2 years ago

That's right, Skaffold would just use options.BuildOptions, not the CLI flags.

So how about this:

As a result of this, if a user specifies -trimpath in their .ko.yaml build config, then Trimpath has no effect. Trimpath being false doesn't remove an explicitly added -trimpath flag.

This way we don't change the behavior of --disable-optimizations, and we don't introduce a new CLI flag.

Does this sound ok? Thanks both of you for your feedback and your help in shaping this. 👍

jonjohnsonjr commented 2 years ago

SGTM