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/compile: strange use of GOEXPERIMENT #22223

Closed
rsc opened this issue Oct 12, 2017 · 7 comments

Comments

Projects
None yet
6 participants
@rsc
Copy link
Contributor

commented Oct 12, 2017

cmd/compile/internal/gc/main.go looks at $GOEXPERIMENT and if it's non-empty disables concurrent compilation.
I can't tell why. GOEXPERIMENT is only meaningful during make.bash; after that the experiment set is baked into the toolchain and not reloaded from the environment.

I suspect this use of $GOEXPERIMENT should just be removed.

/cc @josharian @mdempsky

@rsc rsc added this to the Go1.10 milestone Oct 12, 2017

@mdempsky

This comment has been minimized.

Copy link
Member

commented Oct 12, 2017

It looks like there's a similar check in cmd/go/internal/work/build.go too.

I think the intent was that @josharian wasn't confident the non-standard code paths enabled by experiments were safe for concurrency.

Agreed the checks do not function as intended.

@griesemer griesemer self-assigned this Nov 29, 2017

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

If trivial, do for 1.10. Otherwise move to 1.11.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 29, 2017

Change https://golang.org/cl/80760 mentions this issue: cmd/compile: fix GOEXPERIMENT checks

@gopherbot gopherbot closed this in bd983a6 Dec 1, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

Reopening this, because GOEXPERIMENT is now broken since that CL.

I left comments on https://go-review.googlesource.com/c/go/+/80760 with details.

/cc @rsc

@bradfitz bradfitz reopened this Dec 4, 2017

@bradfitz bradfitz added the NeedsFix label Dec 4, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Dec 4, 2017

Change https://golang.org/cl/81776 mentions this issue: cmd/go: disable concurrent compilation under GOEXPERIMENTs

gopherbot pushed a commit that referenced this issue Dec 4, 2017

cmd/go: disable concurrent compilation under GOEXPERIMENTs
Duplicate cmd/compile check into cmd/go. Manually tested that
"GOEXPERIMENT=fieldtrack make.bash" passes now.

Updates #22223.

Change-Id: I441970a8a5ad4aadf5bd4fbd4d6cc71847b43308
Reviewed-on: https://go-review.googlesource.com/81776
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.11 Dec 6, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2017

As far as I can tell this is a valid issue, but there is no trivial fix, and things are working today, so rolling this forward to 1.11.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

The original issue (that cmd/compile was erroneously checking the GOEXPERIMENT environment variable) is fixed, so I think this can be closed again. (I saw @bradfitz's comment on the CL, but didn't notice he reopened this issue.)

@mdempsky mdempsky closed this Dec 6, 2017

@golang golang locked and limited conversation to collaborators Dec 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.