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: require build systems to specify -importcfg #51225
Comments
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.) |
This proposal has been added to the active column of the proposals project |
I'd like to drop support for |
Just noting that test/run.go passes It might be hard to get rid of |
Ack, but I expect we could reasonably change it to use
Similarly, I expect cmd/go can implement that itself on top of |
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. |
Change https://go.dev/cl/415235 mentions this issue: |
Change https://go.dev/cl/415236 mentions this issue: |
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. |
Those CLs look good. Let's try it for Go 1.19 - they are easily reverted. |
Based on the discussion above, this proposal seems like a likely accept. |
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>
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>
Change https://go.dev/cl/417534 mentions this issue: |
No change in consensus, so accepted. |
…lags Updates #51225. Change-Id: I820f3f5ba169635fee37c30e41b370c9399a436d Reviewed-on: https://go-review.googlesource.com/c/go/+/417534 Reviewed-by: Russ Cox <rsc@golang.org>
Change https://go.dev/cl/420419 mentions this issue: |
Change https://go.dev/cl/420614 mentions this issue: |
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>
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>
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>
…lags Updates golang#51225. Change-Id: I820f3f5ba169635fee37c30e41b370c9399a436d Reviewed-on: https://go-review.googlesource.com/c/go/+/417534 Reviewed-by: Russ Cox <rsc@golang.org>
Is #56776 related to this issue ? If it is, then I think this behavior change will bring a lot of inconvenience, I often use |
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.
The text was updated successfully, but these errors were encountered: