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

vet: Don't use GOROOT to set PATH if GOROOT is unset #7761

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

arjan-bal
Copy link
Contributor

The runner is installing go1.22, but starts using go1.23 when running vet:

PATH=/home/runner/go/bin:/bin:/home/runner/go/bin:/opt/hostedtoolcache/go/1.22.8/x64/bin:/snap/bin:/home/runner/.local/bin:/opt/pipx_bin:/home/runner/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
+ go version
go version go1.23.2 linux/amd64

I added logs to debug the go version before vet.sh updates the PATH

+ go version
go version go1.22.8 linux/amd64
+ which go
/opt/hostedtoolcache/go/1.22.8/x64/bin/go
+ trap cleanup EXIT
+ PATH=/home/runner/go/bin:/bin:/home/runner/go/bin:/opt/hostedtoolcache/go/1.22.8/x64/bin:/snap/bin:/home/runner/.local/bin:/opt/pipx_bin:/home/runner/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
+ go version
go version go1.23.2 linux/amd64
+ which go
/bin/go

The problem is that GOROOT is not set and we're using it to set PATH.

Example failure: https://github.com/grpc/grpc-go/actions/runs/11409299053/job/31749221269?pr=7759

RELEASE NOTES: None

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.70%. Comparing base (98959d9) to head (ce4ea0e).
Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7761      +/-   ##
==========================================
- Coverage   79.75%   79.70%   -0.06%     
==========================================
  Files         365      365              
  Lines       36362    36362              
==========================================
- Hits        29001    28982      -19     
- Misses       6170     6189      +19     
  Partials     1191     1191              

see 15 files with indirect coverage changes

@arjan-bal arjan-bal requested review from easwars and dfawley October 21, 2024 05:28
@purnesh42H purnesh42H changed the title vet: Don't adding GOROOT to PATH if GOROOT is unset vet: Don't use GOROOT for PATH if GOROOT is unset Oct 21, 2024
@purnesh42H purnesh42H changed the title vet: Don't use GOROOT for PATH if GOROOT is unset vet: Don't use GOROOT to set PATH if GOROOT is unset Oct 21, 2024
Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. We can wait for someone from SVL to double check this

@easwars
Copy link
Contributor

easwars commented Oct 21, 2024

Example failure: grpc/grpc-go/actions/runs/11409299053/job/31749221269?pr=7759

Am I reading this failure log correctly?

I see this at the top

+ git status --porcelain
+ fail_on_output
+ tee /dev/stderr
+ not read
+ read
+ trap cleanup EXIT

If git status --porcelain failed, why did it continue to do other things?

@easwars
Copy link
Contributor

easwars commented Oct 21, 2024

The problem is that GOROOT is not set and we're using it to set PATH.

Do we know when this happens? setup-go does seem to set this up here: https://github.com/actions/setup-go/blob/d0c5defdf364f1d1fb07530c000084836192af9c/src/main.ts#L29

@dfawley
Copy link
Member

dfawley commented Oct 21, 2024

I see this at the top

Nothing in there looks like a failure to me? Those are the commands it's running and it doesn't output anything, meaning fail_on_output didn't see any output IIUC.

@easwars
Copy link
Contributor

easwars commented Oct 21, 2024

Nothing in there looks like a failure to me?

TIL what trap cleanup EXIT line actually does.

@arjan-bal
Copy link
Contributor Author

arjan-bal commented Oct 22, 2024

The problem is that GOROOT is not set and we're using it to set PATH.

Do we know when this happens? setup-go does seem to set this up here: https://github.com/actions/setup-go/blob/d0c5defdf364f1d1fb07530c000084836192af9c/src/main.ts#L29

We're using setup-go v5. These seem to be lines responsible for setting GOROOT:

      // Go versions less than 1.9 require GOROOT to be set
      if (semver.lt(version, '1.9.0')) {
        core.info('Setting GOROOT for Go version < 1.9');
        core.exportVariable('GOROOT', installDir);
      }

It seems like GOROOT is not set for recent Go versions.

PR that made the change to not export GOROOT from 2 years ago: actions/setup-go#175

@arjan-bal arjan-bal merged commit c538c31 into grpc:master Oct 22, 2024
15 checks passed
@arjan-bal arjan-bal deleted the fix-vet branch October 22, 2024 17:04
@purnesh42H purnesh42H added Type: Meta Github repo, process, etc Area: Tooling Includes anything related to Go builds, modules etc and includes Releases & Github Workflows. and removed Type: Testing labels Oct 29, 2024
misvivek pushed a commit to misvivek/grpc-go that referenced this pull request Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Tooling Includes anything related to Go builds, modules etc and includes Releases & Github Workflows. Type: Meta Github repo, process, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants