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

x/build/cmd/release: macOS installer should add GOBIN to PATH #39531

Open
jayconrod opened this issue Jun 11, 2020 · 8 comments
Open

x/build/cmd/release: macOS installer should add GOBIN to PATH #39531

jayconrod opened this issue Jun 11, 2020 · 8 comments
Labels
Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jayconrod
Copy link
Contributor

jayconrod commented Jun 11, 2020

We currently provide installers for Go for Windows (an .msi file) and macOS (a .pkg file).

Both installers add $GOROOT/bin to PATH. This lets users run commands like go build without having to edit PATH themselves.

However, users still need to edit PATH in order to run binaries installed with go install or go get. This introduces unnecessary confusion for new developers. To avoid this, both installers should add $GOBIN to PATH as well.

(Note that neither GOBIN nor GOPATH are likely to be set when an installer runs. If they are set, we should respect them; the goal is just to ensure that the directory where go install writes binaries is added to PATH).

@jayconrod jayconrod added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 11, 2020
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Jun 11, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jun 11, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Jun 11, 2020

I think this is a good idea as it will lead to a noticeable improvement to the user experience for new users, and we should do this, unless we are able to discover a reason why doing so could be harmful or dangerous.

Given the installers already own the decision of where to place the Go installation and make it available (via an added entry to PATH or a platform-equivalent), and by not setting a GOPATH, they are implicitly relying on the default GOPATH value that go uses (which is $HOME/go or platform-equivalent), I think also adding GOBIN to PATH is an appropriate and in-scope task for an installer.

I'll think more about it, but I can't think of concerns with doing so.

I'll tentatively mark this for consideration for 1.15, since it seems like a minor and safe change to me (and the change doesn't need to be done in the main Go tree). It's an opportunity to use an upcoming beta release to try it out and get user feedback. That said, we are in the release freeze for 1.15, so if people think this decision needs more time or input, we can move this for 1.16 cycle.

/cc @golang/osp-team

@dmitshur dmitshur modified the milestones: Unreleased, Go1.15 Jun 11, 2020
@jayconrod jayconrod modified the milestones: Go1.15, Go1.16 Jun 15, 2020
@dmitshur dmitshur added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jul 27, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Aug 12, 2020

This issue is currently labeled as early-in-cycle for Go 1.16.
That time is now, so this is a friendly ping so the issue is looked at again.

@dmitshur
Copy link
Contributor

dmitshur commented Sep 24, 2020

@jayconrod Friendly ping on this. Does it sound good to you to move this issue to NeedsFix for 1.16 milestone?

I'll also /cc @stevetraut who's been working on improving the installation experience, I think this change can be beneficial.

@jayconrod jayconrod added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 24, 2020
@jayconrod
Copy link
Contributor Author

jayconrod commented Sep 24, 2020

Sure, it sounds like we're in agreement this is a good idea.

Does anyone on the release team have bandwidth to fix this for 1.16? I'm not familiar with the installers, and I'm not sure if I'll have bandwidth to look at this before the freeze.

@dmitshur
Copy link
Contributor

dmitshur commented Sep 24, 2020

Yes, I can take this.

@dmitshur dmitshur self-assigned this Sep 24, 2020
@dmitshur
Copy link
Contributor

dmitshur commented Sep 24, 2020

It seems this has already been done for Windows 3 years ago. See issue #23025 and CL 104115.

Going to look into the current state of the macOS installer next. Edit: It doesn't have this change yet.

@dmitshur
Copy link
Contributor

dmitshur commented Feb 11, 2021

I've made progress on the side of the macOS installer, but ran into some usability questions around edge cases that we'll need to agree on answers to. I'll provide more detail and follow up on this during the 1.17 cycle.

@dmitshur dmitshur changed the title x/build/cmd/release: installers should add GOBIN to PATH x/build/cmd/release: macOS installer should add GOBIN to PATH Feb 11, 2021
@dmitshur dmitshur modified the milestones: Go1.16, Go1.17 Feb 11, 2021
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. early-in-cycle A change that should be done early in the 3 month dev cycle. labels Feb 11, 2021
@dmitshur dmitshur added this to Planned in Go Release Team Feb 11, 2021
@dmitshur
Copy link
Contributor

dmitshur commented May 11, 2021

I spent some time investigating this and found some additional constraints and challenges. It seems we can't make a simple change here without potentially needing larger UI and behavior changes, and corresponding updates to the installation/uninstallation instructions.

There are the following relevant factors:

  1. The macOS installer, in contrast to the Windows installer, doesn't have ownership over the GOPATH environment variable; it leaves it to be completely user controlled.
  2. The macOS installer, by default, doesn't get access to the same environment a user may run go from.

Ownership of GOPATH

There's agreement it's clear what to do when an installer runs on a clean system, where neither GOPATH nor GOBIN are set. Such cases are likely when installing Go for the first time, and the user hasn't created a custom configuration. In this case, the macOS installer should ensure the default location for user installed Go binaries, as can be determined with $(go env GOPATH)/bin) and currently resolves to "$HOME/go/bin", is added to PATH, so it's possible to invoke foo after doing go install example.org/cmd/foo@latest.

The Windows installer differs from macOS installer in that Windows sets GOPATH to "%USERPROFILE%\go", while macOS one doesn't set it (and rely on cmd/go's default "$HOME/go" one).

The Windows uninstaller has a mechanism to remove the environment variables it sets:

Note: Using this uninstall process for Windows will automatically remove windows environment variables created by the original installation.

The macOS uninstall instructions are to remove /usr/local/go and the /etc/paths.d/go file (which is where $GOROOT/bin is added to users' $PATH).

In the original issue, @jayconrod suggested:

(Note that neither GOBIN nor GOPATH are likely to be set when an installer runs. If they are set, we should respect them; the goal is just to ensure that the directory where go install writes binaries is added to PATH).

I explored that path:

# Take existing $GOBIN and/or $GOPATH
# into account, then add $GOBIN to $PATH.
if _, ok := os.LookupEnv("GOBIN"); ok {
  echo "$GOBIN" >> /etc/path.d/go
} else {
  GOPATH=$(go env GOPATH)
  if $GOPATH == "" {
    // This should never happen. Bail out gracefully if it does.
    return
  }
  $GOBIN := filepath.Join(filepath.SplitList($GOPATH)[0], "bin")
  echo "$GOBIN" >> /etc/path.d/go
}

But came to the conclusion that this might actually be counter-productive:

# TODO(dmitshur): Think more about what to do if GOBIN and GOPATH are already
# set to a custom value; maybe it's better to consider that out of scope for
# the installer (if the user has set a custom $GOBIN OR $GOPATH, it's likely
# they are an advanced user and have already added it to their $PATH, meaning
# the installer shouldn't try to do its own thing)?

So the macOS installer doesn't currently set the GOPATH environment variable, it uses the default one, or the one the user has specified. This is how it behaved for a long time, so suddenly overwriting a potential custom GOPATH without prompting the user would be too disruptive. Starting to prompt the user would add cognitive overhead, so it's a bigger change to consider.

Access to environment configuration

Discussion above assumes it's possible for the installer to know what GOPATH and GOBIN are set to. This means it needs access both to the users environment variables, and also the files that go env modifies via go env -w. At this stage I'm unsure how viable it is, and how reliable it can be made.

Consider that the user may have those environment variables conditionally configured in a .bash_profile or .zprofile. Perhaps it's possible to determine the default shell, and invoke it in a way that creates a default environment, then figure out the configuration from there. I didn't explore this further.

Conclusion

Based on these findings, it seems making this change in the macOS installer may be a bigger undertaking than we anticipated, and needs a more holistic approach.

I considered one possible best-effort solution of appending $HOME/go/bin to /etc/path.d/go regardless, which may do the right thing in most cases. However, there'd be no way to opt out of this behavior, and it might be unwelcome to users whose GOPATH is intentionally elsewhere. I decided against it.

While talking about this briefly with @toothrot, he suggested a possible idea for the cmd/go command to consider printing a prompt after go install example.com/cmd@latest if it detects that the installed cmd isn't in already PATH. If false positives can be eliminated (i.e., some users may intentionally choose not add GOPATH/bin to PATH), this could be similar to some other helpful messages cmd/go prints in some situations.

I don't plan to keep working on this for now, so moving out the Go 1.17 milestone.

@dmitshur dmitshur removed this from Planned in Go Release Team May 11, 2021
@dmitshur dmitshur modified the milestones: Go1.17, Backlog May 11, 2021
@dmitshur dmitshur removed their assignment May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants