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) ?

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

this is correct, my directory contains a .git directory

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.

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.

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.

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.

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.

@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.

@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.

@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 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.

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.

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.

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.

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).

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

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).

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

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.

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.

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.

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.

@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.

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.

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

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