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: VCS stamping complains inappropriately about VCS in unreleated parent directory #49004

Closed
rogpeppe opened this issue Oct 15, 2021 · 20 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Milestone

Comments

@rogpeppe
Copy link
Contributor

commit 1cbec68

The Go compiler refuses to build my usual executables without specifying the -buildvcs=false flag.

Here's a testscript reproducer:

mkdir d/.bzr
cd d/m
exec git init
go build

-- d/m/go.mod --
module m

go 1.18

-- d/m/main.go --
package main

func main() {
}

This fails as follows:

% testscript /tmp/build-bug.txtar

> mkdir d/.bzr
> cd d/m
$WORK/d/m
> exec git init
[stdout]
Initialized empty Git repository in $WORK/d/m/.git/
> go build
[stderr]
error obtaining VCS status: directory "$WORK/d/m" uses git, but parent "$WORK/d" uses bzr
	Use -buildvcs=false to disable VCS stamping.

[exit status 1]
FAIL: /tmp/testscript742157310/build-bug.txtar/script.txt:4: unexpected go command failure
error running /tmp/build-bug.txtar in /tmp/testscript742157310/build-bug.txtar

When the nearest VCS directory is at the root of the current module, I believe it should be taken as definitive and the search should go no further back into parent directories.

@seankhliao
Copy link
Member

shouldn't this be posted in #37475 which tracks the implementation?

cc @jayconrod

@jayconrod jayconrod added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 15, 2021
@jayconrod jayconrod added this to the Go1.18 milestone Oct 15, 2021
@jayconrod
Copy link
Contributor

cc @bcmills @matloob

This is somewhat working as intended: I reused some code from GOPATH go get, which forbids repository-within-repository, except Git-within-Git, which is allowed. In general though, there's a lot of ambiguity where repositories, modules, packages, and directories don't correspond, so it seemed better to report an error if anything was usual.

Could you say more about your use case? Would be good to understand how common this will be.

@bcmills
Copy link
Member

bcmills commented Oct 15, 2021

I agree that if the VCS containing the CWD matches the VCS containing the module root, we shouldn't fail due to other VCS directories further up.

The reason they were rejected in GOPATH-mode go get was to avoid VCS-injection attacks, because in GOPATH mode the go command runs its VCS commands in arbitrary directories that aren't directly controlled by the user. In this case, the user does control the path (it's always the CWD).

@rogpeppe
Copy link
Contributor Author

My use case is that I happen to have a catch-all .bzr directory in my home. I probably shouldn't (it's a historical accident) but nonetheless I have never had any problems it ever before, and I don't think I should now.

I appreciate that VCS within VCS is a specific issue when there's a VCS directory that's below the module root, but I don't think there should be a problem in this case.

AFAICS the logic should be something like: find the nearest enclosing VCS directory:; if it doesn't enclose the root of the current module, it's an error; otherwise use the revision from it.

I can't currently think of a reason to look outside the nearest enclosing VCS directory.

@bcmills bcmills self-assigned this Oct 15, 2021
@gopherbot
Copy link

Change https://golang.org/cl/356309 mentions this issue: cmd/go: allow nested VCS repositories when preparing build stamp

@williamh
Copy link

One of my users on Gentoo is getting bitten by this because he has a fossil control directory in /var.
The sandbox violation means that go is trying to do things outside the package build staging area.
Apparently this is causing a number of go-based packages to not build any more.

https://bugs.gentoo.org/836261

As you can see, he is recommending that we disable the function for builds within our package manager. What do you suggest?

@berndf
Copy link

berndf commented Mar 28, 2022

Thanks @williamh ! I'm that gentoo user. I have to say that, during each build, firing up software that will ascend the whole directory tree looking for control directories that may or may not be project related is probably not a good idea. Not everybody compiles using pristine machine images... Chances are that version information of that unrelated project may be used. And many people will have several VCS installed, meaning that the search will happen using git, hg, bzr, fossil and whatnot, increasing the attack or failure surface a lot.

I do see that sometimes a go project can be in a subdirectory of a larger projects' root. One way to avoid the problem would be to explicitly specify the VCS root in the build. Or to place a "stop VCS search here" file or directory in the project's root. Or, at least to never fire up the VCS search if a version tag is given explicitly (as in gentoo I believe it is).

One specific problem with fossil might also be that it opens the repository read/write even with only informational commands. Not sure that all the other systems open theirs readonly in this case. Sandbox systems would probably abort the build in this case as well, as the repository directory will often be out of bounds. So, without one of the provisions mentioned above avoiding the directory ascending altogether, each and every VCS would need a sandbox-safe "probe mode". This probably means asking too much and we're back to adding more logic to safely conclude that 1) we need info from the VCS and 2) we're actually inside a relevant project-specific checkout.

What can be done immediately is, of course, to set -buildvcs=false if we know that we're not building from a checkout but some tar release. Are there any objections to that?

@seankhliao
Copy link
Member

There is going to be a new default of -buildvcs=auto (ref #51748) though I don't think it's going to help in your case. VCS info also includes clena/dirty info, not just version tags. If you know you're building from a source tarball that isn't going to contain vcs information, setting -buildvcs=false sounds like a reasonable default for your situation.

@williamh
Copy link

That means all package manager builds in Gentoo will end up having -buildvcs=false since we build from tarballs.
I can add that to GOFLAGS once I verify that it will not break go 1.17 since we keep the two most recent versions of go.

@mvdan
Copy link
Member

mvdan commented Mar 28, 2022

@williamh unfortunately Go 1.17 will try to consume GOFLAGS=-buildvcs=false and fail, as that version doesn't know the flag.

@williamh
Copy link

Unfortunately I just discovered this.
I'm going to think about what we can do on our side, but this is definitely a breaking change.
I maintain a basic framework that allows other gentoo developers to write packages for software written in Go, and one thing it does is set global GOFLAGS. The issue I'm going to have until go 1.17 is removed from Gentoo (which will happen when go 1.19 is released) is that I need to set those global GOFLAGS in a way that works for go 1.17 and 1.18 without knowing which version of Go is installed, or I need to find a way to know which version is installed, which I don't see right now.
I will update here, and if you have any suggestions other than not setting -buildvcs=false, that would be great.

@mvdan
Copy link
Member

mvdan commented Mar 28, 2022

If you can in any way control the environment where go build is called, perhaps you could hide the VCS programs like git from $PATH, or hide the VCS directory via e.g. chroot. I understand those aren't as easy as export GOFLAGS=..., but at least they wouldn't break Go 1.17.x.

@mvdan
Copy link
Member

mvdan commented Mar 28, 2022

Another potential idea: set up $PATH with a go wrapper that first calls the underlying go version; if it contains the string go1.17, export GOFLAGS=-buildvcs=false. Then, call the underlying Go normally via exec go "$@".

@williamh
Copy link

A wrapper might be an option, but it would need to be a wrapper that only the package manager can use. I don't really want to change the behavior of the go command outside the package manager context.
I'll explore this idea.

valyala added a commit to VictoriaMetrics/VictoriaMetrics that referenced this issue May 11, 2022
… builds

This should resolve the `error obtaining VCS status: exit status 128` error
when the environment contains incorrect version of git or has incorrect access rights
to the directory with VictoriaMetrics source code.

See the following links for additional info:
- #2508 (comment) ,
- ko-build/ko#672
- golang/go#49004
valyala added a commit to VictoriaMetrics/VictoriaMetrics that referenced this issue May 11, 2022
… builds

This should resolve the `error obtaining VCS status: exit status 128` error
when the environment contains incorrect version of git or has incorrect access rights
to the directory with VictoriaMetrics source code.

See the following links for additional info:
- #2508 (comment) ,
- ko-build/ko#672
- golang/go#49004
hagen1778 pushed a commit to VictoriaMetrics/VictoriaMetrics that referenced this issue May 12, 2022
* deployment/docker: pass `-buildvs=false` to `go build` for production builds

This should resolve the `error obtaining VCS status: exit status 128` error
when the environment contains incorrect version of git or has incorrect access rights
to the directory with VictoriaMetrics source code.

See the following links for additional info:
- #2508 (comment) ,
- ko-build/ko#672
- golang/go#49004

* lib/netutil: limit the number of concurrently established connections when calling ConnPool.Get()

This should reduce potential spikes in the number of established connections in the following cases:
- when the connection establishing procedure becomes temporarily slow
- after a temporary spike in the rate of ConnPool.Get() calls

See #2552

* docs/CHANGELOG.md: document c8af625

See #1322 (comment)

* docs/Cluster-VictoriaMetrics.md: typo fix: `by by` -> `by`

* docs: add `resource usage limits` docs, which describe fine-grained tuning for various resource usage limits

* docs/Cluster-VictoriaMetrics.md: the `/api/v1/label/.../values` query can take CPU and ram at both vmstorage and vmselect

* Update root Readme and root vmagent readme

Co-authored-by: Aliaksandr Valialkin <valyala@victoriametrics.com>
valyala added a commit to VictoriaMetrics/VictoriaMetrics that referenced this issue May 13, 2022
* deployment/docker: pass `-buildvs=false` to `go build` for production builds

This should resolve the `error obtaining VCS status: exit status 128` error
when the environment contains incorrect version of git or has incorrect access rights
to the directory with VictoriaMetrics source code.

See the following links for additional info:
- #2508 (comment) ,
- ko-build/ko#672
- golang/go#49004

* lib/netutil: limit the number of concurrently established connections when calling ConnPool.Get()

This should reduce potential spikes in the number of established connections in the following cases:
- when the connection establishing procedure becomes temporarily slow
- after a temporary spike in the rate of ConnPool.Get() calls

See #2552

* docs/CHANGELOG.md: document c8af625

See #1322 (comment)

* docs/Cluster-VictoriaMetrics.md: typo fix: `by by` -> `by`

* docs: add `resource usage limits` docs, which describe fine-grained tuning for various resource usage limits

* docs/Cluster-VictoriaMetrics.md: the `/api/v1/label/.../values` query can take CPU and ram at both vmstorage and vmselect

* Update root Readme and root vmagent readme

Co-authored-by: Aliaksandr Valialkin <valyala@victoriametrics.com>
AndrewChubatiuk pushed a commit to AndrewChubatiuk/VictoriaMetrics that referenced this issue May 13, 2022
… builds

This should resolve the `error obtaining VCS status: exit status 128` error
when the environment contains incorrect version of git or has incorrect access rights
to the directory with VictoriaMetrics source code.

See the following links for additional info:
- VictoriaMetrics#2508 (comment) ,
- ko-build/ko#672
- golang/go#49004
AndrewChubatiuk pushed a commit to AndrewChubatiuk/VictoriaMetrics that referenced this issue May 13, 2022
* deployment/docker: pass `-buildvs=false` to `go build` for production builds

This should resolve the `error obtaining VCS status: exit status 128` error
when the environment contains incorrect version of git or has incorrect access rights
to the directory with VictoriaMetrics source code.

See the following links for additional info:
- VictoriaMetrics#2508 (comment) ,
- ko-build/ko#672
- golang/go#49004

* lib/netutil: limit the number of concurrently established connections when calling ConnPool.Get()

This should reduce potential spikes in the number of established connections in the following cases:
- when the connection establishing procedure becomes temporarily slow
- after a temporary spike in the rate of ConnPool.Get() calls

See VictoriaMetrics#2552

* docs/CHANGELOG.md: document c8af625

See VictoriaMetrics#1322 (comment)

* docs/Cluster-VictoriaMetrics.md: typo fix: `by by` -> `by`

* docs: add `resource usage limits` docs, which describe fine-grained tuning for various resource usage limits

* docs/Cluster-VictoriaMetrics.md: the `/api/v1/label/.../values` query can take CPU and ram at both vmstorage and vmselect

* Update root Readme and root vmagent readme

Co-authored-by: Aliaksandr Valialkin <valyala@victoriametrics.com>
@rsc rsc unassigned bcmills Jun 23, 2022
@MichaelVoelkel
Copy link

MichaelVoelkel commented Jun 30, 2022

Sorry, this is where google leads with the error and this thread is a bit cluttered...

So, the solution is to just use go build -buildvcs=false . when using in a Docker container, e.g.? Worked for me, at least, but some word about general recommendation for dummies could be helpful. (As said, Google leads here, would be a cool nice-to-have)

@mvdan
Copy link
Member

mvdan commented Jul 1, 2022

This particular bug was about nested VCS repositories causing an error - that was solved, and no workaround is needed.

I assume the problem you have is that git may not be installed inside your Docker image, where you run go build inside a git directory. That is #51748, which is fixed in 1.18.2, so it does not need a workaround either.

@MichaelVoelkel
Copy link

Thanks for your answer!
I tried both using golang:1.18.2-buster instead of golang:1.18.1-buster and installing git via apt-get in my container before but I still get:

go: downloading github.com/nats-io/nats.go v1.14.0
go: downloading github.com/nats-io/nkeys v0.3.0
go: downloading github.com/nats-io/nuid v1.0.1
go: downloading golang.org/x/crypto v0.0.0-20220315160706-3147a52a75dd
error obtaining VCS status: exit status 128
	Use -buildvcs=false to disable VCS stamping.

But -buildcvs=false still works for me

@seankhliao
Copy link
Member

Please file a new bug with steps to reproduce.

@MichaelVoelkel
Copy link

Did it with sample nested repo: #53640 (repo: https://github.com/MichaelVoelkel/vcs-test )

ryukinix added a commit to ryukinix/keto that referenced this issue Aug 19, 2022
How to replicate the error: `make docker`

Previous error:
```
... long log
 ---> f394d136bb6c
Step 7/24 : ADD proto/go.sum proto/go.sum
 ---> 7fca7e520cb3
Step 8/24 : ENV CGO_ENABLED 1
 ---> Running in ca87bf346155
Removing intermediate container ca87bf346155
 ---> af2dbef0c762
Step 9/24 : RUN go mod download
 ---> Running in 98785997fa9c
Removing intermediate container 98785997fa9c
 ---> f74577fb6d14
Step 10/24 : ADD . .
 ---> f1cc0ac767f0
Step 11/24 : RUN go build -tags sqlite -o /usr/bin/keto .
 ---> Running in 913258959d81
error obtaining VCS status: exit status 128
	Use -buildvcs=false to disable VCS stamping.
The command '/bin/sh -c go build -tags sqlite -o /usr/bin/keto .'
returned a non-zero code: 1
```

This is fixed by setting -buildvcs=false

ref: golang/go#49004
zepatrik pushed a commit to ryukinix/keto that referenced this issue Aug 24, 2022
How to replicate the error: `make docker`

Previous error:
```
... long log
 ---> f394d136bb6c
Step 7/24 : ADD proto/go.sum proto/go.sum
 ---> 7fca7e520cb3
Step 8/24 : ENV CGO_ENABLED 1
 ---> Running in ca87bf346155
Removing intermediate container ca87bf346155
 ---> af2dbef0c762
Step 9/24 : RUN go mod download
 ---> Running in 98785997fa9c
Removing intermediate container 98785997fa9c
 ---> f74577fb6d14
Step 10/24 : ADD . .
 ---> f1cc0ac767f0
Step 11/24 : RUN go build -tags sqlite -o /usr/bin/keto .
 ---> Running in 913258959d81
error obtaining VCS status: exit status 128
	Use -buildvcs=false to disable VCS stamping.
The command '/bin/sh -c go build -tags sqlite -o /usr/bin/keto .'
returned a non-zero code: 1
```

This is fixed by setting -buildvcs=false

ref: golang/go#49004
@Southclaws
Copy link

Just ran into this on GitHub Actions + Fly.io no idea what vcs is or why it only happened in that specific environment (fly.io manual deploy works fine, GitHub Actions can also build Docker images fine, but if I invoked fly deploy from a GitHub action, this confusing error appears)

So, just commenting here with some keywords to help others who may be searching for that exact combination of tools - because I couldn't find anything!

Also gonna ping @michaeldwan just in case this is something the Fly team can fix in their environment for Go applications :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker
Projects
None yet
Development

No branches or pull requests

10 participants