Skip to content
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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: possible race condition when applying templates to flags/ldflags #913

Merged
merged 2 commits into from
Dec 22, 2022
Merged

Conversation

caarlos0
Copy link
Contributor

@caarlos0 caarlos0 commented Dec 21, 2022

while working on goreleaser/goreleaser#3653, I got some race conditions sometimes.

Seems to happen when the base image is a image index, in which case it'll build for all platform needed concurrently, which is when we get the concurrent writes.

I don't see a big issue in returning a copy instead, not sure what you think.

PS: Its late here, so please pardon me if I'm doing something dumb 馃槀

Signed-off-by: Carlos A Becker <caarlos0@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2022

Codecov Report

Merging #913 (1d05794) into main (ac2df47) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #913      +/-   ##
==========================================
- Coverage   51.34%   51.30%   -0.05%     
==========================================
  Files          44       44              
  Lines        3414     3417       +3     
==========================================
  Hits         1753     1753              
- Misses       1432     1435       +3     
  Partials      229      229              
Impacted Files Coverage 螖
pkg/build/gobuild.go 56.97% <0.00%> (-0.21%) 猬囷笍

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@caarlos0 caarlos0 marked this pull request as ready for review December 21, 2022 04:30
@caarlos0
Copy link
Contributor Author

giving it a second though, a question: when the base image is not multi-platform, should ko still try to build for all the given platforms?

@imjasonh
Copy link
Member

giving it a second though, a question: when the base image is not multi-platform, should ko still try to build for all the given platforms?

ko's behavior here is kind of weird, since there are a number of scenarios it has to cover.

  1. If the base image is a single-platform image, --platform basically only describes the platform to set on the output image. KO_DEFAULTBASEIMAGE=some-amd64-image ko build --platform=linux/arm64 even works, and adds an arm64 Go binary on top of the amd64 base. 馃檭
  2. --platform=all when building on a single-platform base just means "whatever the base image's platform is", which is nearly always linux/amd64, and might just always default to that (I haven't tried; there really aren't a lot of single-platform non-amd64 images).

I think we should keep this behavior (at least (2)), because of how it interacts with ko resolve where there may be many builds and many base images. You'd want ko resolve --platform=all to produce multi-arch images when possible, and single-arch images when that's all that's available in the base.

@imjasonh imjasonh merged commit ff69e17 into ko-build:main Dec 22, 2022
@caarlos0 caarlos0 deleted the race branch December 23, 2022 11:57
@caarlos0
Copy link
Contributor Author

ah cool, thanks for explaining all that, @imjasonh!

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants