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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: add (and default to) -buildvcs=auto #51748

Closed
mschneider82 opened this issue Mar 17, 2022 · 30 comments
Closed

cmd/go: add (and default to) -buildvcs=auto #51748

mschneider82 opened this issue Mar 17, 2022 · 30 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mschneider82
Copy link

I build my projects with a basic Dockerfile based on golang:alpine, the project dependencies are in the /vendor directory, so no additional download is required. (e.g. when the build containers are not connected to the internet)

it also uses the flag:
ENV GOFLAGS -mod=vendor

this worked well before 1.18 (with an older version of golang:alpine):

docker build -t myservice .

Sending build context to Docker daemon  108.2MB
Step 1/16 : FROM golang:alpine as builder
 ---> 193418aa4321
Step 2/16 : RUN mkdir -p /go/src/the/path/myservice
 ---> Running in 05a96bf34d09
Removing intermediate container 05a96bf34d09
 ---> 9521959475d3
Step 3/16 : ADD . /go/src/the/path/myservice/
 ---> 9422e1bcf73e
Step 4/16 : WORKDIR /go/src/the/path/myservice
 ---> Running in 5e6ba7885d39
Removing intermediate container 5e6ba7885d39
 ---> 5c017e30587c
Step 5/16 : ENV GOFLAGS -mod=vendor
 ---> Running in 49c7bafa12c1
Removing intermediate container 49c7bafa12c1
 ---> 709db29e8cbf
Step 6/16 : RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o myservice .
 ---> Running in 95fa26e0baba
Removing intermediate container 95fa26e0baba
 ---> 3dd723395943
Step 7/16 : FROM alpine
 ---> e50c909a8df2
Step 8/16 : RUN adduser -S -D -H -h /app appuser
 ---> Using cache
 ---> 4e72bf554722
Step 9/16 : RUN mkdir -p /app/config
 ---> Using cache
 ---> 94fe18c448fd
Step 10/16 : COPY --from=builder /go/src/the/path/myservice/config.yml /app/config
 ---> 8d9678bac41f
Step 11/16 : COPY --from=builder /go/src/the/path/myservice/myservice /app/
 ---> d2634c5cd75c
Step 12/16 : RUN chown appuser /app -R
 ---> Running in ad86f7ce8c8f
Removing intermediate container ad86f7ce8c8f
 ---> 003a2a823362
Step 13/16 : USER appuser
 ---> Running in e499ca060a6f
Removing intermediate container e499ca060a6f
 ---> 3bcc8ec26edf
Step 14/16 : WORKDIR /app
 ---> Running in ce3e9b1c4bd8
Removing intermediate container ce3e9b1c4bd8
 ---> d298a549af1a
Step 15/16 : LABEL some=label
 ---> Running in 63233dfdd0c2
Removing intermediate container 63233dfdd0c2
 ---> b1a2212ac232
Step 16/16 : ENTRYPOINT ["./myservice"]
 ---> Running in e9f0389e5bb3
Removing intermediate container e9f0389e5bb3
 ---> ef3ec44b2fce
Successfully built ef3ec44b2fce
Successfully tagged myservice:latest

after i did docker pull golang:alpine it now fails because it tries to download the dependencies with git (which is also not installed in the alpine image):

Sending build context to Docker daemon  108.2MB
Step 1/16 : FROM golang:alpine as builder
 ---> f25c27b88763
Step 2/16 : RUN mkdir -p /go/src/the/path/myservice
 ---> Using cache
 ---> 9f221460c293
Step 3/16 : ADD . /go/src/the/path/myservice/
 ---> 91908022017e
Step 4/16 : WORKDIR /go/src/the/path/myservice
 ---> Running in 4bfd07048c2c
Removing intermediate container 4bfd07048c2c
 ---> 048d0c9d4bf8
Step 5/16 : ENV GOFLAGS -mod=vendor
 ---> Running in 1b16198b759d
Removing intermediate container 1b16198b759d
 ---> b240205af17d
Step 6/16 : RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o myservice .
 ---> Running in 540afc754acb
go: missing Git command. See https://golang.org/s/gogetcmd
error obtaining VCS status: exec: "git": executable file not found in $PATH
        Use -buildvcs=false to disable VCS stamping.
The command '/bin/sh -c CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o myservice .' returned a non-zero code: 1

doing a go mod vendor does not change anything, any advice?

@mschneider82
Copy link
Author

mschneider82 commented Mar 17, 2022

setting buildvcs=false solved this issue. Should I keep this open since it is a breaking change that was not mentioned in the release notes (in part of go build) ?

@seankhliao seankhliao changed the title go build with vendor fails on 1.18 in docker alpine cmd/go: build requires git for buildvcs Mar 17, 2022
@seankhliao seankhliao added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go labels Mar 17, 2022
@seankhliao
Copy link
Member

This is unrelated to vendoring, and is probably triggered by the existence of a .git dir.

cc @bcmills @matloob

Not sure if this should be folded into #51723

@mschneider82
Copy link
Author

this is correct, my directory contains a .git directory

@mvdan
Copy link
Member

mvdan commented Mar 17, 2022

I don't think this is a duplicate; my issue is about go test, where presumably -buildvcs should never be needed.

This is about go build, which I understand is meant to default to -buildvcs=true. So I think this change is by design. I see how it can break some user scripts, but I don't think go build should behave differently based on whether git is installed - that would make reproducible builds quite a bit harder, and the documentation more confusing. If you really don't need VCS information to be stamped, you can always omit or delete the .git folder when building, or supply -buildvcs=false.

@tie
Copy link
Contributor

tie commented Mar 17, 2022

I think the current -buildvcs=true default already makes reproducible builds harder since the output depends not only on the source tree but the whole project history. For example, a build from tarball release source tree would not be identical to Git build, unless the whole Git history is included in tarball, for an otherwise identical source trees.

I’ve added -buildvcs=false to GOFLAGS environment variable (in addition to -trimpath, which, honestly, should be enabled by default instead) on all my machines when this change was introduced, but that’s after I’ve faced this issue on CI where Git was not installed—luckily, a few hours after the commit so I’ve figured it out quickly, but I can see how this can be frustrating with user scripts that’ve been working for years.

Ideally, go build should use VCS information to make the output identical to go install example.com@version (assuming that the tree is not dirty). Though that seems to be a non-trivial change, and I see how current -buildvcs=true behavior may be useful for projects that build releases from VCS source tree. That said, I’d rather not have it enabled by default.

@mvdan
Copy link
Member

mvdan commented Mar 17, 2022

for an otherwise identical source trees.

I think they key word there is "otherwise" :) The source trees aren't identical.

I don't have strong feelings on what the default for go build -buildvcs should be, but at least I can say that the new default is intentional and clearly documented in https://go.dev/doc/go1.18#go-command.

If you want your Go builds to be fully reproducible regardless of details like the presence of VCS or the current directory, you already have to supply flags like -trimpath, so I don't think this change should make reproducible builds significantly harder. There's also others like CGO_ENABLED=1 being a default for builds targetting the host platform, where you also have to worry about which C compiler you're using.

@mvdan
Copy link
Member

mvdan commented Mar 17, 2022

I don't have strong feelings on what the default for go build -buildvcs should be

I will add one reason why the default being true makes sense, though. If a user of a program of mine does git clone [...]; cd project; go build and runs into a bug, I want them to be able to see the version information, so they can give it to me. The average user will not have advanced knowledge of how go build works, and they'll simply send me a bug report with the missing information. It's a lot easier if the default behavior includes the information.

The other case is reproducible builds, like you say, where the presence or state of the VCS can cause problems. My argument in the previous comment is that doing a fully reproducible Go build requires a bit of knowledge and care, so the average person doing that will already know what flags to supply and how to stay up to date with changes to flags or default behavior in go help build. So I think it's a minimal nuisance for them to add -buildvcs=false to their list of flags, if that's what they want - though I would argue that a reproducible build should still include version information somehow.

tklauser added a commit to tklauser/numcpus that referenced this issue Mar 17, 2022
Explicitly disable buildvcs for build and test on FreeBSD. See
golang/go#51723 and
golang/go#51748.
@ocket8888
Copy link

I don't much care whether vcs info is included in builds by default, but there was a breaking change in that my go build commands on systems without git used to succeed and they now fail. go build can try to use git to put in vcs info, but if it can't find git on the system it shouldn't fail the build. It should just do whatever it used to do. That would make this a non-breaking change.

@mvdan
Copy link
Member

mvdan commented Mar 17, 2022

@ocket8888 that would make go build silently produce a different binary depending on whether a program is installed locally, which has never been the case before, as far as I'm aware.

@DasSkelett
Copy link

@ocket8888 that would make go build silently produce a different binary depending on whether a program is installed locally, which has never been the case before, as far as I'm aware.

In this case it default to -buildvcs=false to avoid this problem, and if it's explicitly enabled throwing an error is fine and expected.

@mvdan
Copy link
Member

mvdan commented Mar 17, 2022

@DasSkelett then see #51748 (comment). I again don't have a strong opinion here, but this is not a matter of simply changing the default behavior.

@ocket8888
Copy link

ocket8888 commented Mar 17, 2022

@ocket8888 that would make go build silently produce a different binary depending on whether a program is installed locally, which has never been the case before, as far as I'm aware.

Well, it now produces either an entire binary or nothing at all based on whether a program is installed locally, which is worse IMO.

RE: #51748 (comment),

If a user of a program of mine does git clone [...]; cd project; go build and runs into a bug, I want them to be able to see the version information, so they can give it to me

In this situation the user has already demonstrated at least basic knowledge of what is basically the world standard in version control systems. By contrast I had no idea Go put vcs info in its binaries until my builds started failing. I don't even know how to extract that information from a Go binary - but I do know how to git log. I can't imagine the reverse being true for anyone, so I would argue you should be able to get that information from them either way.

the average person [doing fully reproducible Go builds] will already know what flags to supply and how to stay up to date with changes to flags or default behavior in go help build.

To be clear, this wasn't simply a change in default behavior, it was - at the very least from the perspective of go help build - the addition of a new build-time requirement that previously couldn't be enabled or disabled at all:

$ go help build | grep vcs | wc -l
0
$ go version
go version go1.17.8 linux/amd64

I think it's a minimal nuisance for them to add -buildvcs=false to their list of flags, if that's what they want

It is minimal, but it's still a nuisance. Backward compatibility for go isn't guaranteed, I realize, but breaking changes should still be kept to a minimum IMO because people who haven't read through the "Go compatibility" docs page (which btw for some reason isn't linked to from http://go.dev/doc/) it can come as a bit of an annoying shock when a minor version introduces a breaking interface change.

tklauser added a commit to tklauser/numcpus that referenced this issue Mar 17, 2022
Explicitly disable buildvcs for build and test on FreeBSD. See
golang/go#51723 and
golang/go#51748.
@seankhliao
Copy link
Member

I had no idea Go put vcs info in its binaries until my builds started failing.

It's new, so not knowing it would be embedded is normal, but the build fails with very clear instructions.

I don't even know how to extract that information from a Go binary - but I do know how to git log.

An executable isn't always used on the system or by the person who built it, and nothing guarantees the binary you kept around is at the same version as your source tree if you're actively working on something.

I think the default should remain as is, on a minimal system there are already other things you have to disable (eg set CGO_ENABLED=0) and having the information available by default will be more beneficial to the applications that want to report this info.

@mvdan
Copy link
Member

mvdan commented Mar 17, 2022

Well, it now produces either an entire binary or nothing at all based on whether a program is installed locally, which is worse IMO.

I guess it depends on what you prioritize, but I think go build needs to be as predictable and easy to understand as possible. Giving you a clear error when it wants to use git and it isn't installed is clear; silently producing a different binary can lead to confusion.

I fully empathize with some go build commands working with 1.17 and breaking with 1.18. I've had two of those in my projects. But at the end of the day, soft breakages like these (in the sense that there's an easy workaround or upgrade path) are sometimes necessary to get new features. It happened gradually with the introduction of modules, for instance. Just look at the amount of support #37475 had. And the proposal was always for it to be the new default, just like stamping module version information, and not opt-in.

And, to reiterate, there are three options which I hope are reasonable:

  1. Do you care about VCS info? Install git.
  2. Do you not? Supply -buildvcs=false. You can even set it globally via export GOFLAGS=-buildvcs=false.
  3. If you don't control what calls go build, you can always remove or hide the .git folder, e.g. via a .dockerignore line.

@ocket8888
Copy link

ocket8888 commented Mar 17, 2022

Well like I said I don't care about the default behavior one way or the other, since existing build systems that don't utilize it would be none the wiser. I just don't think a failure to find git should result in a compilation failure.

silently producing a different binary can lead to confusion.

It shouldn't do that silently, I would definitely expect to see a warning on stderr when some feature fails to work.

And, to reiterate, there are three options which I hope are reasonable

This isn't a huge deal. It's a little annoying, but the fix will take me all of one second to implement. I do find the statement "soft breakages ... are sometimes necessary to get new features" a bit ironic for a team/tool/ecosystem that are of the opinion "there is no good way to way to make breaking changes to a library or a tool", "Breaking changes are bad", etc. :P . But no, this isn't a hill I'm ready to die on (and trust me there are more than enough of those already), those are pretty simple fixes/workarounds. I do still think a warning is a better UX than a failure here, but not strongly enough to continue arguing in its favor.

@DasSkelett
Copy link

DasSkelett commented Mar 17, 2022

It shouldn't do that silently, I would definitely expect to see a warning on stderr when some feature fails to work.

I wouldn't like that, and it most likely won't happen, the stance on compiler warnings in the Go team appears to be:

There are two reasons for having no warnings. First, if it's worth complaining about, it's worth fixing in the code. (And if it's not worth fixing, it's not worth mentioning.) Second, having the compiler generate warnings encourages the implementation to warn about weak cases that can make compilation noisy, masking real errors that should be fixed.

https://go.dev/doc/faq#unused_variables_and_imports

A warning here isn't useful and just adds noise. The only way to fix the warning is to disable the feature, so you might as well fail the build and require the same (which is, in the end, just a one time change in the CI scripts). Warnings tend to be ignored if they are even noticed at all (nobody looks at the CI logs if everything's green, and this problem is unlikely to occur on developer machines).

@mschneider82
Copy link
Author

mschneider82 commented Mar 18, 2022

if the folder .git is not found it silently compiles without vcs.
if the folder .git was found but the command git is not found it fails to compile.

for my point of view I would expect it skips vcs and compiles because vcs is not forced to be present. But i am also okay with the breaking change, we had a few in the last years using go build for example the introduction of -mod=vendor

@bcmills
Copy link
Contributor

bcmills commented Mar 18, 2022

I see three possible options here:

  1. Leave -buildvcs as-is. (Always stamp VCS info when inside a repo, and users who don't want to install git for their git repo can set -buildvcs=false, either explicitly or in GOFLAGS, or delete the .git directory when pushing to CI.)

  2. Implicitly disable version stamping when the VCS tool is missing even if -buildvcs=true.

  3. Make the default -buildvcs value some third option (auto?). Require a VCS tool for -buildvcs=true, and implicitly disable stamping when the tool is missing for -buildvcs=auto.

Personally I'm kind of leaning toward (3).

@bcmills bcmills added this to the Go1.19 milestone Mar 18, 2022
@bcmills bcmills self-assigned this Mar 18, 2022
@ocket8888
Copy link

I would find (3) acceptable, for whatever that's worth.

@mvdan
Copy link
Member

mvdan commented Mar 18, 2022

I oppose 2, in terms of easier reproducibility. I prefer 1 over 3 in terms of less "auto" magic, but it's not a strong conviction; either would still solve my problem, since the average user running git clone [...] go build will still have the VCS installed.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #51798 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@bhcleek
Copy link
Contributor

bhcleek commented Mar 30, 2022

I think there may be a further complication. By relying on git status, any .gitattributes files come into play regardless of whether the files are otherwise used during compilation, preventing go build from succeeding because of a missing filter or diff.

This could happen, for instance, if there is a .gitattributes file that specifies a diff and/or filter attribute for a file that isn't otherwise involved in the build process. Users could potentially workaround it by providing the respective filter/diff as a no-op, but that seems overly cumbersome and error prone, because the particulars of the no-op filter/diff that needs to be provided to be able to use go build -buildvcs=true will vary widely.

I can imagine -buildvcs=auto working as -buildvcs=false when git exists and git status fails, but that doesn't seem appropriate, because git status may fail for other reasons, too.

@mpx
Copy link
Contributor

mpx commented Mar 31, 2022

There are likely numerous obscure ways buildvcs/git could fail on existing repos, or fail in future. It might be better for auto to simply disable buildvcs when it is unable to retrieve the required input. Eg, git status or git show fails. Anyone who wants to guarantee buildvcs succeeds can use --buildvcs=true.

Effectively it a choice between:

  • Bugs preventing compilation for a relatively uncommon/obscure reason (most people are unfamiliar with buildvcs and the interaction w/VCS tooling)
  • Bugs to enable buildvcs when it skips tagging for a relatively uncommon/obscure reason.

I think the later is a better choice. It would have prevented several of the breakages seen during the development of buildvcs.

@bcmills
Copy link
Contributor

bcmills commented Apr 7, 2022

@mpx, you are correct that it would have prevented several of the failure modes that we saw (and you fixed — thanks again!) during development, but on the other hand we would not have seen those failure modes and known to fix them, which I think would have left us in a worse state overall — we would have been left with more situations in which the VCS metadata is unexpectedly, silently, and (in some sense) inexplicably missing from the resulting binary.

I think -buildvcs=auto surfacing errors from the VCS tool is probably the right balance: it avoids the common failure mode (missing the tool entirely), while still reporting the more obscure errors so they can be fixed — and it still allows the explicit -buildvcs=false workaround to allow users to unblock themselves if they decide that the failure really is benign.

@ocket8888
Copy link

Personally, if go was my tool, I would make it emit a warning in the case that --buildvcs=auto is used (implicitly or explicitly) and proceed without vcs info, and with --buildvcs=true I'd make it error. But if you want to avoid warnings entirely, I think @bcmills is right and it's better to let those errors bubble up rather than be lost to the void.

Benjamintf1 pushed a commit to cloudfoundry/syslog-release that referenced this issue Apr 7, 2022
- letting the pipeline do the actual golang bump and tests

- go tries to include git information but stemcells do not include git binary
  golang/go#51748

Signed-off-by: Andrew Crump <crumpan@vmware.com>
Benjamintf1 pushed a commit to cloudfoundry/windows-syslog-release that referenced this issue Apr 7, 2022
- letting the pipeline do the actual golang bump and tests

- go tries to include git information but stemcells do not include git binary
  golang/go#51748

- go install instead of go get for installing in go 1.18

Signed-off-by: Ben Fuller <benjaminf@vmware.com>
Co-authored-by: Ben Fuller <benjaminf@vmware.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/398855 mentions this issue: cmd/go: allow '-buildvcs=auto' and treat it as the default

Benjamintf1 pushed a commit to cloudfoundry/windows-syslog-release that referenced this issue Apr 13, 2022
* fix for go 1.18

- letting the pipeline do the actual golang bump and tests

- go tries to include git information but stemcells do not include git binary
  golang/go#51748

- go install instead of go get for installing in go 1.18

Signed-off-by: Ben Fuller <benjaminf@vmware.com>
Co-authored-by: Ben Fuller <benjaminf@vmware.com>

* increasing the timeouts seems to help?

Co-authored-by: Andrew Crump <crumpan@vmware.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/400454 mentions this issue: [release-branch.go1.18] cmd/go: allow '-buildvcs=auto' and treat it as the default

jinnatar added a commit to jinnatar/chia_exporter that referenced this issue Apr 18, 2022
If the .git directory is included, the build fails on a new golang
feature that naively assumes a build environment with a .git folder has
git installed.
gopherbot pushed a commit that referenced this issue Apr 20, 2022
…s the default

When we added VCS stamping in the Go 1.18 release, we defaulted to
-buildvcs=true, on the theory that most folks will actually want VCS
information stamped.

We also made -buildvcs=true error out if a VCS directory is found and
no VCS tool is available, on the theory that a user who builds with
'-buildvcs=true' will be very surprised if the VCS metadata is
silently missing.

However, that causes a problem for CI environments that don't have the
appropriate VCS tool installed. (And we know that's a common situation
because we're in that situation ourselves — see #46693!)

The new '-buildvcs=auto' setting provides a middle ground: it stamps
VCS information by default when the tool is present (and reports
explicit errors if the tool errors out), but omits the metadata
when the tool isn't present at all.

Updates #51748.
Updates #51999.
Fixes #51798.

Change-Id: Iebc955c2af0abca9b7517f62ca48b1d944eb2df4
Reviewed-on: https://go-review.googlesource.com/c/go/+/398855
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 4569fe6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/400454
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
@rsc rsc unassigned bcmills Jun 22, 2022
@golang golang locked and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests