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/go: executables are sometimes named after their module's last element, not directory [1.12 backport] #30266

Closed
gopherbot opened this issue Feb 15, 2019 · 12 comments
Assignees
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 15, 2019

@mvdan requested issue #27283 to be considered for backport to the next 1.12 minor release.

@gopherbot please backport to 1.12

@julieqiu

This comment has been minimized.

Copy link

@julieqiu julieqiu commented Mar 12, 2019

@mvdan - there isn't a reason provided in the gopherbot message. Would you mind providing one for this backport?

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Mar 13, 2019

See Filippo's comment in the original thread.

Is there a convention to add the reason for the backport in the backport issue directly?

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 13, 2019

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

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 13, 2019

Closed by merging 746edd4 to release-branch.go1.12.

@gopherbot gopherbot closed this Mar 13, 2019
gopherbot pushed a commit that referenced this issue Mar 13, 2019
… versioned binaries

`go build` has chosen the last element of the package import path
as the default output name when -o option is given. That caused
the output of a package build when the module root is the major
version component such as 'v2'.

A similar issue involving `go install` was fixed in
https://golang.org/cl/128900. This CL refactors the logic added
with the change and makes it available as
internal/load.DefaultExecName.

This CL makes 'go test' to choose the right default test binary
name when the tested package is in the module root. (E.g.,
instead of v2.test, choose pkg.test for the test of 'path/pkg/v2')

Fixes #27283
Fixes #30266

Change-Id: I6905754f0906db46e3ce069552715f45356913ae
Reviewed-on: https://go-review.googlesource.com/c/go/+/140863
Reviewed-by: Bryan C. Mills <bcmills@google.com>
(cherry picked from commit bf94fc3)
Reviewed-on: https://go-review.googlesource.com/c/go/+/167384
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Mar 14, 2019

Is there a convention to add the reason for the backport in the backport issue directly?

@mvdan Yes. I'll quote from https://golang.org/wiki/MinorReleases (emphasis mine):

As soon as an interested party thinks an issue should be considered for backport, they open one or two “child” issues titled like package: title [1.9 backport]. The issue should include a link to the original issue and a short rationale about why the backport might be needed.

When asking gopherbot to backport an issue, the reason should be included in that message. That
way it ends up in the backport issue description (e.g., see #30266 (comment)), and that helps with making the decision of whether a cherry-pick candidate issue should be accepted. Thanks!

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 14, 2019

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
…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 <andybons@golang.org>
@bcmills bcmills reopened this Mar 14, 2019
@gopherbot gopherbot closed this Mar 14, 2019
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 14, 2019

Closed by merging 32355f5 to release-branch.go1.12.

@bcmills bcmills reopened this Mar 14, 2019
@andybons andybons modified the milestones: Go1.12.1, Go1.12.2 Mar 14, 2019
@katiehockman

This comment has been minimized.

Copy link
Member

@katiehockman katiehockman commented Mar 19, 2019

@bcmills why was this re-opened? It looks like the cherry pick to the 1.12 branch was already merged. I'm going to re-label and close it now, but re-open if there is something else still pending.

@katiehockman

This comment has been minimized.

Copy link
Member

@katiehockman katiehockman commented Mar 19, 2019

Ah I see it was reverted at https://go-review.googlesource.com/c/go/+/167617

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Mar 22, 2019

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

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Mar 22, 2019

Approving this because it's a regression from GOPATH mode. There was an attempt to get it into 1.12.1 that didn't work out, so we're trying again for 1.12.2.

gopherbot pushed a commit that referenced this issue Mar 27, 2019
… 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 <jayconrod@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
(cherry picked from commit 94563de)
Reviewed-on: https://go-review.googlesource.com/c/go/+/168958
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Mar 27, 2019

Closed by merging aeb9d03 to release-branch.go1.12.

(I'm closing instead of @gopherbot because it doesn't handle this edge case right now, see issue #31094.)

@dmitshur dmitshur closed this Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.