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

x/build/cmd/release: leave oldlink binary out of RC1 (if the beta doesn't uncover serious linker issues) #39509

Closed
toothrot opened this issue Jun 10, 2020 · 9 comments
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@toothrot
Copy link
Contributor

toothrot commented Jun 10, 2020

As part of https://golang.org/s/better-linker, we now build a binary called oldlink in our release of Go. While it may be useful to include in the beta, we should make a decision on whether this is included in the RC or final releases of Go 1.15, vs asking people to rebuild it on their own if necessary.

/cc @cherrymui @aclements @golang/osp-team

@toothrot toothrot added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels Jun 10, 2020
@toothrot toothrot added this to the Go1.15 milestone Jun 10, 2020
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Jun 10, 2020
@aclements
Copy link
Member

/cc @jeremyfaller @thanm

I think it's a good idea to include this in the beta. While our internal testing gets good coverage of certain configurations, it doesn't get very good coverage of others, and shipping this in the beta will help with any problems uncovered there.

However, the new linker roll out has been going really smoothly so far. Assuming the beta doesn't uncover any serious linker issues, I'd be inclined to leave oldlink out of the RC.

@toothrot toothrot self-assigned this Jun 10, 2020
@cherrymui
Copy link
Member

@aclements 's suggestion SGTM.

A test may fail if the old linker is not present. We could change the test to skip if the binary does not exist.

@thanm
Copy link
Contributor

thanm commented Jun 10, 2020

I'm happy with @aclements 's suggestion too.

@aclements
Copy link
Member

It was really @toothrot 's suggestion, so I think we're all in agreement. :)

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jun 17, 2020
@dmitshur dmitshur changed the title x/build/cmd/release: decide whether to include oldlink binary in release x/build/cmd/release: leave oldlink binary out of RC1 (if the beta doesn't uncover serious linker issues) Jun 18, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Jul 14, 2020

A test may fail if the old linker is not present. We could change the test to skip if the binary does not exist.

@cherrymui Were you referring to a specific test? Or did you mean we should look for such tests and update them as needed?

I can try to find anything by taking go1.15beta1, removing the oldlink binary, then seeing if anything comes up during go test std cmd.

@cherrymui
Copy link
Member

@cherrymui Were you referring to a specific test?

TestOldLink in cmd/link.

@dmitshur
Copy link
Contributor

dmitshur commented Jul 14, 2020

Thanks!

This is how the test fails when oldlink binary isn't present:

$ go test -v -run=TestOldLink -count=1 cmd/link
=== RUN   TestOldLink
    link_test.go:558: [/tmp/tmp.AG3oE0Ne/go/bin/go run -gcflags=all=-go115newobj=false -asmflags=all=-go115newobj=false -ldflags=-go115newobj=false /tmp/TestOldLink814981397/main.go]: exit status 2:
        # command-line-arguments
        2020/07/14 18:36:02 invoke oldlink failed:fork/exec /tmp/tmp.AG3oE0Ne/go/pkg/tool/darwin_amd64/oldlink: no such file or directory
--- FAIL: TestOldLink (0.14s)
FAIL
FAIL	cmd/link	0.269s

It's going to affect people who try to run go test cmd/link after installing a binary release of Go from https://golang.org/dl (but not anyone who builds from source via make.bash or all.bash).

Given that 1.15 is shipping a new linker, it's better to avoid misleading test failures, so skipping the test if oldlink binary isn't available seems like a good solution.

@gopherbot
Copy link

Change https://golang.org/cl/242604 mentions this issue: cmd/link: skip TestOldLink if the old linker does not exist

@toothrot
Copy link
Contributor Author

The binary was removed from releases in https://golang.org/cl/242643, but the commit message incorrectly failed to mention the repository to reference this issue.

@golang golang locked and limited conversation to collaborators Jul 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants