Skip to content

cmd/go/internal/load: filepath.Split used on a '/'-separated path #30821

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.

Sign up for GitHub

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
Closed
dmitshur opened this issue Mar 14, 2019 · 4 comments
Closed

cmd/go/internal/load: filepath.Split used on a '/'-separated path #30821

dmitshur opened this issue Mar 14, 2019 · 4 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.13

Comments

@dmitshur
Copy link
Member

On master right now:

go/src/cmd/go/internal/load/pkg.go

Line 1192 in 1024b25

_, elem := filepath.Split(p.ImportPath)

Import path is always '/'-separated, but filepath.Split operates on OS-defined file paths that may not be '/'-separated.

This was introduced as part of CL 140863, I don't think it was intentional. See my comment there for details:

https://go-review.googlesource.com/c/go/+/140863/11#message-db0ff6bb2c7b06161ca47de771c4465afa8b1102

/cc @hyangah @bcmills @bradfitz
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. GoCommand cmd/go labels Mar 14, 2019
@dmitshur dmitshur added this to the Go1.13 milestone Mar 14, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/167503 mentions this issue: cmd/go: use path.Split on import path in DefaultExecName

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/167617 mentions this issue: Revert "[release-branch.go1.12] cmd/go: fix the default build output name for versioned binaries"

gopherbot pushed a commit that referenced this issue Mar 14, 2019
Revert "[release-branch.go1.12] cmd/go: fix the default build output …
32355f5 
…name for versioned binaries"

This reverts commit 746edd4 (CL 167384).

Reason for revert: Dmitri identified a potential problem in https://go-review.googlesource.com/c/go/+/140863/11#message-db0ff6bb2c7b06161ca47de771c4465afa8b1102, and we'd like more time to investigate without holding up the 1.12 release branch.

Updates #27283
Updates #30266
Updates #30821

Change-Id: I49d7bbbe200e80b81899c3bcbf7844717af010aa
Reviewed-on: https://go-review.googlesource.com/c/go/+/167617
Reviewed-by: Andrew Bonventre <[email protected]>
@dmitshur dmitshur self-assigned this Mar 14, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/168958 mentions this issue: [release-branch.go1.12] cmd/go: fix the default build output name for versioned binaries

@dmitshur
Copy link
Member Author

For posterity, I made CL 167618 where I intentionally simulated what would happen on an operating system where forward slash (/) is not a recognized filepath separator, to see if there are any tests to catch that.

It failed because the go binary did not get installed with the right name (it had a name like cmd/go.exe instead of just go.exe), so it didn't get very far:

Building Go cmd/dist using C:\workdir\go1.4
Building Go toolchain1 using C:\workdir\go1.4.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for windows/amd64.
go tool dist: FAILED: C:\workdir\go\bin\go list -gcflags=all= -ldflags=all= -f={{if .Stale}}	STALE {{.ImportPath}}: {{.StaleReason}}{{end}} std cmd: exec: "C:\\workdir\\go\\bin\\go": file does not exist
The system cannot find the batch label specified - fail


Error: build failed: make script failed: exit status 1

(Source: https://storage.googleapis.com/go-build-log/a564c9ab/windows-amd64-2016_1e739c43.log.)

gopherbot pushed a commit that referenced this issue Mar 27, 2019
@dmitshur
[release-branch.go1.12] cmd/go: fix the default build output name for…
aeb9d03 
… versioned binaries

This change is a re-apply of the reverted CL 140863 with changes to
address issue #30821. Specifically, path.Split continues to be used
to split the '/'-separated import path, rather than filepath.Split.

Document the algorithm for how the default executable name is determined
in DefaultExecName.

Rename a variable returned from os.Stat from bs to fi for consistency.

CL 140863 factored out the logic to determine the default executable
name from the Package.load method into a DefaultExecName function,
and started using it in more places to avoid having to re-implement
the logic everywhere it's needed. Most previous callers already computed
the default executable name based on the import path. The load.Package
method, before CL 140863, was the exception, in that it used the p.Dir
value in GOPATH mode instead. There was a NOTE(rsc) comment that it
should be equivalent to use import path, but it was too late in Go 1.11
cycle to risk implementing that change.

This is part 1, a more conservative change for backporting to Go 1.12.2,
and it keeps the original behavior of splitting on p.Dir in GOPATH mode.
Part 2 will address the NOTE(rsc) comment and modify behavior in
Package.load to always use DefaultExecName which splits the import path
rather than directory. It is intended to be included in Go 1.13.

Updates #27283
Updates #26869
Updates #30821
Fixes #30266

Change-Id: Ib1ebb95acba7c85c24e3a55c40cdf48405af34f3
Reviewed-on: https://go-review.googlesource.com/c/go/+/167503
Reviewed-by: Jay Conrod <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
(cherry picked from commit 94563de)
Reviewed-on: https://go-review.googlesource.com/c/go/+/168958
Run-TryBot: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
@golang golang locked and limited conversation to collaborators Mar 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
2 participants
@dmitshur @gopherbot