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

Jump to bottom

cmd/compile: require build systems to specify -importcfg #51225

Open
mdempsky opened this issue Feb 16, 2022 · 16 comments
Open

cmd/compile: require build systems to specify -importcfg #51225

mdempsky opened this issue Feb 16, 2022 · 16 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Proposal Proposal-Accepted
Projects
Proposals (old)
Milestone
Backlog

Comments

@mdempsky
Copy link
Member

Originally, cmd/compile implemented a lot of logic for resolving import paths, finding installed packages, etc. internally. Nowadays, this is all handled by cmd/go instead, which then invokes cmd/compile with the -importcfg option. However, a lot of historical code and flags still exist within cmd/compile, and it's not obvious which use cases still matter.

The proposal here is to require build systems that want predictable behavior from cmd/compile must use -importcfg. If -importcfg is not used, then cmd/compile's handling of import paths is unspecified and subject to change at the discretion of the compiler team.

In practice, I don't anticipate the fallback logic to change much from today (to avoid breaking test/run.go or compiler dev workflows). But the current code has grown convoluted and has questionable code paths, and it would be easier to cleanup if we could constrain the set of supported options.
@mdempsky mdempsky added this to the Go1.19 milestone Feb 16, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Feb 16, 2022
@rsc
Copy link
Contributor

rsc commented Jun 22, 2022

What are the specific changes you are proposing?

I think in practice it is already true that non-importcfg behavior is undefined. So I'm happy to say that.

But I'm not sure what it accomplishes to just say that. It seems like if we want to do this, we need to put teeth behind it, by making the compiler fail without -importcfg. That will break most people working on the compiler running things like “go tool compile” by hand, which seems like a mistake. (We could adjust test/run.go, but the by-hand stuff still worries me a little.)

@rsc rsc modified the milestones: Go1.19, Proposal Jun 22, 2022
@rsc
Copy link
Contributor

rsc commented Jun 22, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Jun 22, 2022
@mdempsky
Copy link
Member Author

I'd like to drop support for -D, -I, and -importmap. Maybe -installsuffix too. I'd have to double check, but I don't believe any of these flags are needed when -importcfg is used.

@ianlancetaylor
Copy link
Contributor

Just noting that test/run.go passes -D and -I to cmd/compile,

It might be hard to get rid of -installsuffix since it's also a option for go build. But perhaps nobody uses it.

@mdempsky
Copy link
Member Author

Just noting that test/run.go passes -D and -I to cmd/compile,

Ack, but I expect we could reasonably change it to use -importcfg instead.

It might be hard to get rid of -installsuffix since it's also a option for go build.

Similarly, I expect cmd/go can implement that itself on top of -importcfg and -o, if it doesn't already. (I'd expect the build cache effectively requires cmd/go to do this actually, but maybe not.)

@rsc
Copy link
Contributor

rsc commented Jun 29, 2022

Assuming that Bazel uses -importcfg and not the other flags, this seems fine. But it's not enough to say they're deprecated: we should either do nothing or remove them. Do you want to update all.bash not to depend on them?

In the absence of real data, we could try leaving the implementations in place for Go 1.19 but add just a few easily reverted lines to fail any compile using the flags and see how the RC goes.

@gopherbot
Copy link

Change https://go.dev/cl/415235 mentions this issue: cmd/compile: remove -importmap flag

@gopherbot
Copy link

Change https://go.dev/cl/415236 mentions this issue: cmd/{compile,link}: remove -installsuffix flags

@mdempsky
Copy link
Member Author

Above two CLs trivially remove cmd/compile's -importmap and -installsuffix flags without any all.bash breakage, and I think are pretty minimally invasive. (cmd/link has an -installsuffix flag too, but it seems to actually be needed for something; this is outside the scope of my concerns here though.)

Removing -D and -I will require at a minimum some more invasive changes to various tests. I'm wary of attempting that for Go 1.19, but I think they're doable for Go 1.20.

@rsc
Copy link
Contributor

rsc commented Jul 13, 2022

Those CLs look good. Let's try it for Go 1.19 - they are easily reverted.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Jul 13, 2022
@rsc
Copy link
Contributor

rsc commented Jul 13, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

gopherbot pushed a commit that referenced this issue Jul 13, 2022
@mdempsky
cmd/compile: remove -importmap flag
1355ea3 
Obsoleted by -importcfg, and no longer used by anything.

Updates #51225.

Change-Id: I49e646d2728347f862f90805051bb03dd4f4bed2
Reviewed-on: https://go-review.googlesource.com/c/go/+/415235
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
gopherbot pushed a commit that referenced this issue Jul 13, 2022
@mdempsky
cmd/compile: remove -installsuffix flag
558785a 
Obsoleted by -importcfg.

cmd/link has a similar flag, but it seems to still be needed at least
for misc/cgo/testshared.TestGopathShlib. I can't immediately tell why
(has something to do with finding .so files), but it doesn't appear to
possibly affect cmd/compile.

Updates #51225.

Change-Id: I80c6aef860bd162c010ad4a1a4f532b400cf901c
Reviewed-on: https://go-review.googlesource.com/c/go/+/415236
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/417534 mentions this issue: doc: mention removal of cmd/compile's -importmap and -installsuffix flags

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Jul 20, 2022
@rsc
Copy link
Contributor

rsc commented Jul 20, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: cmd/compile: require build systems to specify -importcfg cmd/compile: require build systems to specify -importcfg Jul 20, 2022
@rsc rsc modified the milestones: Proposal, Backlog Jul 20, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 20, 2022
gopherbot pushed a commit that referenced this issue Aug 1, 2022
@mdempsky @rsc
doc: mention removal of cmd/compile's -importmap and -installsuffix f…
8b13a07 
…lags

Updates #51225.

Change-Id: I820f3f5ba169635fee37c30e41b370c9399a436d
Reviewed-on: https://go-review.googlesource.com/c/go/+/417534
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/420419 mentions this issue: doc: remove mention of -installsuffix removal

@gopherbot
Copy link

Change https://go.dev/cl/420614 mentions this issue: _content/doc: remove mention of -installsuffix removal

gopherbot pushed a commit to golang/website that referenced this issue Aug 1, 2022
@mdempsky
_content/doc: remove mention of -installsuffix removal
3af8cad 
We ended up reverting CL 415236 due to a Google-internal dependency,
but I didn't update the doc fix CL before it was submitted.

Updates golang/go#51225.

Change-Id: I36142e6ea3f15251c54a7c3a6ddb953f9c18775f
Reviewed-on: https://go-review.googlesource.com/c/website/+/420614
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@mdempsky @jproberts
cmd/compile: remove -importmap flag
5ae24f0 
Obsoleted by -importcfg, and no longer used by anything.

Updates golang#51225.

Change-Id: I49e646d2728347f862f90805051bb03dd4f4bed2
Reviewed-on: https://go-review.googlesource.com/c/go/+/415235
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@mdempsky @jproberts
cmd/compile: remove -installsuffix flag
90ff201 
Obsoleted by -importcfg.

cmd/link has a similar flag, but it seems to still be needed at least
for misc/cgo/testshared.TestGopathShlib. I can't immediately tell why
(has something to do with finding .so files), but it doesn't appear to
possibly affect cmd/compile.

Updates golang#51225.

Change-Id: I80c6aef860bd162c010ad4a1a4f532b400cf901c
Reviewed-on: https://go-review.googlesource.com/c/go/+/415236
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@mdempsky @jproberts
doc: mention removal of cmd/compile's -importmap and -installsuffix f…
589c6c5 
…lags

Updates golang#51225.

Change-Id: I820f3f5ba169635fee37c30e41b370c9399a436d
Reviewed-on: https://go-review.googlesource.com/c/go/+/417534
Reviewed-by: Russ Cox <rsc@golang.org>
@erifan
Copy link

erifan commented Nov 17, 2022

Is #56776 related to this issue ? If it is, then I think this behavior change will bring a lot of inconvenience, I often use go tool compile -S to check the generated code, and I think there will be many other users who do the same.

tvlbot pushed a commit to tvlfyi/kit that referenced this issue Feb 12, 2024
@lukegb
chore(nix/buildGo): add support for Go 1.20+
268988b 
golang/go#51225 and other changes mean that importcfgs are now basically required for Go 1.20+; we also separately compile the Go stdlib, since it looks like pkgs.go no longer actually has the compiled version of the stdlib shipped, just the source.

Change-Id: Ibf5ee7d43f7800c6dd1e0dec6c7a6d35ef50b7b0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10801
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Proposal Proposal-Accepted
Projects
Go Compiler / Runtime
Status: Triage Backlog
Proposals
Status: Accepted
Release Status & Release Blockers
Status: No status
Proposals (old)
Accepted
Milestone
Backlog
Development

No branches or pull requests
5 participants
@mdempsky @rsc @ianlancetaylor @gopherbot @erifan