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/dist: allow Go to be built inside of module root #44209

Closed
zx2c4 opened this issue Feb 10, 2021 · 12 comments
Closed

cmd/dist: allow Go to be built inside of module root #44209

zx2c4 opened this issue Feb 10, 2021 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Feb 10, 2021

My build system downloads 1.4, downloads newer source, patches it, and then runs ./make.bash. It does this because I have custom patches that require a full rebuild (I can no longer just rebuild cmd/link or similar). I've automated all of this nicely in my makefile, which downloads tarballs, checks hashes, runs build commands, and ultimately .deps/go/bin/go exists and works. The rest of my build system then runs with export PATH := $(CURDIR)/.deps/go/bin:$(PATH). All is well.

Except for one thing: src/cmd/dist/build.go errors out if you're building in a child directory of something with a go.mod in it: fatalf("found go.mod file in %s: $GOROOT must not be inside a module", modRoot). So that means my makefile has to rename go.mod to go.mod.renamed, or struggle to find a good tmp directory on the same partition for atomic moves or some other type of headache.

Instead, it would be nice to be able to run ./make.bash anywhere, regardless of what the parent directories have in them.

Are there any big architectural reasons why this can't be?

@zx2c4
Copy link
Contributor Author

zx2c4 commented Feb 10, 2021

git-blame indicates that this is actually the result of a sort of meh fix to #36701. So CCing @ianlancetaylor and @jayconrod to see if there's a better way to address this.

@jayconrod
Copy link
Contributor

I don't think there's any architectural reason this can't be done. We've just seen a ton of bizarre errors that come up when GOROOT is nested inside a module or a GOPATH root.

@jayconrod jayconrod added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 10, 2021
@jayconrod jayconrod added this to the Unplanned milestone Feb 10, 2021
@zx2c4
Copy link
Contributor Author

zx2c4 commented Feb 10, 2021

I'm playing with introducing a GO_MOD_CEILING_DIRECTORIES environment variable, with the same semantics as GIT_CEILING_DIRECTORIES. make.{bash,bat,rc} would then just set that variable.

@jayconrod
Copy link
Contributor

I don't think there's any reason cmd/go and cmd/dist can't figure it out automatically; no need to add new environment varaibles. Just need more logic in the right places.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Feb 10, 2021

Alright, I'll drop it then. Are you suggesting logic is added to cmd/dist? Know a good place to start?

@jayconrod
Copy link
Contributor

More likely cmd/go, but I'm not sure where. It's been a while since I worked on this, and it doesn't look like I put much thought into it for CL 215939.

cmd/dist basically contains a primitive form of the go command's import-path-to-directory resolution logic: just enough to build cmd/go itself. It knows about vendor directories, but not modules or GOPATH.

Once cmd/go is built as go_bootstrap, I think everything else should be built in module mode, and that should mostly work regardless of what's in parent directories. I'd expect to run into complications if GO111MODULE=off, and maybe also with the misc tests.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/291069 mentions this issue: internal/modload: do not traverse backwards for go_bootstrap

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/291289 mentions this issue: cmd/dist: allow building in subdirectory of go module

@zx2c4
Copy link
Contributor Author

zx2c4 commented Feb 23, 2021

@jayconrod @dmitshur - We now have two ways of fixing this - https://golang.org/cl/291069 and https://golang.org/cl/291289 . Can you make a decision about which you like better (or neither) so we can move ahead in some direction?

@jayconrod
Copy link
Contributor

Sorry I haven't reviewed these yet. This issue is still on my radar, but I've had a longer than usual review backlog lately.

I'm still hoping this can be solved in cmd/dist without changing cmd/go or adding a go.mod file. Your comment in 291069 surprised me though:

This errors at the stage, "Building Go toolchain2 using go_bootstrap and Go toolchain1", so it's not just the initial step of building cmd/dist.

I'd like to spend a bit of time debugging to make sure I understand why that's the case.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/296610 mentions this issue: dist: run the go tool from src/

@jayconrod
Copy link
Contributor

@zx2c4 Thanks for working on this. We ended up going in a slightly different direction: 1) cmd/dist won't look for go.mod above GOROOT, and 2) when cmd/dist invokes go_bootstrap, it does so in a temporary directory with its own go.mod. So it will find a go.mod file without needing any extra special casing in cmd/go.

We didn't check in a go.mod in GOROOT, but make.bash should tolerate one being there (or in any other parent).

@golang golang locked and limited conversation to collaborators Mar 2, 2022
@dmitshur dmitshur modified the milestones: Unplanned, Go1.17 Aug 9, 2024
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants