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.

Sign up for GitHub

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

Jump to bottom

cmd/go: vcs.* info can be inaccurate for Mercurial #63532

Open
mharbison72 opened this issue Oct 13, 2023 · 6 comments · May be fixed by #63557
Open

cmd/go: vcs.* info can be inaccurate for Mercurial #63532

mharbison72 opened this issue Oct 13, 2023 · 6 comments · May be fixed by #63557
Labels
GoCommand cmd/go help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@mharbison72
Copy link

What version of Go are you using (go version)?

$ go1.20.2 version
go version go1.20.2 windows/amd64

Does this issue reproduce with the latest release?

I didn't try, but looking at the current code, yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go1.20.2 env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Matt\AppData\Local\go-build
set GOENV=C:\Users\Matt\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\Matt\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\Matt\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Users\Matt\sdk\go1.20.2
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Users\Matt\sdk\go1.20.2\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.20.2
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\Matt\AppData\Local\Temp\go-build3808080945=/tmp/go-build -gno-record-gcc-switches

What did you do?

I ran go1.20.2.exe version -m on my executable.

What did you expect to see?

        build   vcs=hg
        build   vcs.revision=76cef81bcff371c88d277f17c712ecf22b8c83e7
        build   vcs.time=2023-10-12T23:09:25Z
        build   vcs.modified=true

What did you see instead?

        build   vcs=hg
        build   vcs.revision=82be5bac456b8fbde0268380e336fe2fbd1294e7
        build   vcs.time=2023-09-28T04:18:18Z
        build   vcs.modified=true

It looks like 76cef81 introduced a regression when it switched from identify -i (which produces information about the current checkout) to log -l1 (which prints info about whatever the topmost commit is, and may be a totally different branch). [1] From the commit comment there, it sounds like this particular change was to get the full commit hash too. The fix here is to simply add a -r. argument to the log command to look at the current commit, and that will fix both vcs.time and vcs.revision.

Typically, the topmost commit is whatever the developer is working on, but it won't be if updating to an old commit, or pulling changes from the server. While these might be edge cases, creating a tag creates a new commit, and CI typically updates to the commit that was tagged. That means this info will be wrong for any tagged builds (which is how I noticed this).

There are two other correctness issues here:

  • The status command should have a -S argument, so that it recurses into any subrepos when checking for modified files.[2]

  • HGPLAIN=1 should be set in the environment when executing any commands, to prevent unexpected command output changes.[3] For example, I have ui.tweakdefaults=True in my configuration, so at the end of hg status listing any file changes, it can print info about the current state of the repo, like this:

# The repository is in an unfinished *bisect* state.

# To mark the changeset good:    hg bisect --good
# To mark the changeset bad:     hg bisect --bad
# To abort:                      hg bisect --reset

Given the current code of checking for any status output, that would incorrectly flag the build as modified.

[1] 76cef81#diff-50574a97a14d32e7cc56ad1ba6de8c554e491c48a3718e599cfc8cc0ab2858e4R166
[2] https://github.com/golang/go/blame/cc13161a0d62fc1deab019996c17a7da1ff6a7f1/src/cmd/go/internal/vcs/vcs.go#L211
[3] https://repo.mercurial-scm.org/hg/file/6.5.2/mercurial/helptext/scripting.txt#l45
@seankhliao seankhliao changed the title runtime/debug: vcs.* info can be inaccurate for Mercurial cmd/go: vcs.* info can be inaccurate for Mercurial Oct 13, 2023
@bcmills bcmills added the GoCommand cmd/go label Oct 13, 2023
@bcmills
Copy link
Contributor

bcmills commented Oct 13, 2023

@mharbison72, given that you seem to understand the problems here, would you like to send a fix?

(I'm not sure that I have a solid enough understanding of the Mercurial command line to do it properly — especially to write a regression test that would correctly reproduce these bugs.)

@bcmills bcmills added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Oct 13, 2023
@bcmills bcmills added this to the Backlog milestone Oct 13, 2023
@mharbison72
Copy link
Author

@mharbison72, given that you seem to understand the problems here, would you like to send a fix?

(I'm not sure that I have a solid enough understanding of the Mercurial command line to do it properly — especially to write a regression test that would correctly reproduce these bugs.)

I'd be happy too, but I haven't contributed here before. I'm reading the contributing doc and need to get setup first.

I'm not sure where you are in the release cycle- is there a particular branch I should target to get these fixes backported? Also, where are the tests I should add to? I see src/cmd/go/internal/vcs/vcs_test.go and src/cmd/go/testdata/vcstest/hg which I assume is what you mean, but I'm not sure how to run them. Is the typical workflow to just push to a PR, and let tests run on GH?

@bcmills
Copy link
Contributor

bcmills commented Oct 13, 2023

I'm not sure where you are in the release cycle- is there a particular branch I should target to get these fixes backported?

All fixes should land on the master branch first; then we can decide about backports.

Also, where are the tests I should add to?

src/cmd/go/testdata/script — see in particular src/cmd/go/testdata/script/version_buildvcs_hg.txt.

They are run as subtests of cmd/go.TestScript, so you can invoke them individually like:

$ go test cmd/go -run=TestScript/version_buildvcs_hg

@mharbison72
Copy link
Author

Thanks for the pointers @bcmills. I'm probably missing something obvious, but the tests seem to sail right over the changes I'm making to version_buildvcs_hg.txt when I run the test command in verbose mode, with caching disabled:

$ time GOFLAGS="-count=1" go test -v cmd/go -run=TestScript/version_buildvcs_hg

Also, if I understand how the scripts work, they aren't currently testing the revision or time value at all, other than that it's present:

 stdout '^\tbuild\tvcs.revision='
 stdout '^\tbuild\tvcs.time='
 stdout '^\tbuild\tvcs.modified=false$'

I can see why that was done- the current time and username are part of the commit hash, so they aren't fixed from run to run. They can be set to specific values in the test environment like in the Mercurial test setup if you want to go that far (and go.exe doesn't clear them somewhere), but I don't want to leave you with a bunch of complexity. Without it though, there's really no way to add regression tests for this.

@mharbison72
Copy link
Author

I figured out my problem- I was running the global go instead of the local one to run the tests.

LMK if the more elaborate revision matching in the tests is what you had in mind, or if you meant something else.

mharbison72 added a commit to mharbison72/go that referenced this issue Oct 15, 2023
@mharbison72
cmd/go: harvest the correct Mercurial changeset information for vcs.*…
1ee53fd 
… stamps

Updates golang#63532

The previous code looked at the most recent commit, not the current checkout.
Now there's not a special case for an empty repo, and there should always be
output from the command.
mharbison72 added a commit to mharbison72/go that referenced this issue Oct 15, 2023
@mharbison72
cmd/go: don't confuse optional Mercurial status output as modified files
ecf5f10 
Updates golang#63532
mharbison72 added a commit to mharbison72/go that referenced this issue Oct 15, 2023
@mharbison72
cmd/go: recurse into Mercurial subrepos when looking for modified files
e0d7bd1 
Fixes golang#63532

I'm not bothering with a test for this, because there's a bit of setup and `-S`
is simply an instruction to not stop at the subrepo boundary.  The command is
otherwise the same.
@mharbison72 mharbison72 linked a pull request Oct 15, 2023 that will close this issue
Open
@gopherbot
Copy link

Change https://go.dev/cl/535377 mentions this issue: cmd/go: fix the accuracy of Mercurial vcs.* stamped data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
GoCommand cmd/go help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

cmd/go: fix the accuracy of Mercurial vcs.* stamped data mharbison72/go
3 participants
@bcmills @gopherbot @mharbison72