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

Add eng/install-scripts/microsoft-go-install.ps1 #182

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dagood
Copy link
Member

@dagood dagood commented Oct 10, 2024

I started with the .NET install script and turned it into a Microsoft Go install script while generally keeping features/scenarios and making it work with tar.gz archives. I kept my changes in a separate commit.

From here, we can decide what we actually want to maintain and perhaps cut a lot.

A feature I didn't include is installing the "correct" Go based on the project. In .NET a global.json may specify a .NET version, in Go we could leverage the go.mod's toolchain go1.23.2 directive. However, there's no room for our revision -1/-2 here, so it wouldn't be totally reproducible. (It would have to download the newest revision we release of the matching 1.x.y.)

I ran this with these (all amd64):

  • Windows:
    • PowerShell (most restrictive)
    • PowerShell Core
  • Linux (Fedora)
    • PowerShell Core

@dagood dagood force-pushed the dev/dagood/pwsh-install-script branch from d04ae78 to 921185a Compare October 17, 2024 18:42
@dagood dagood marked this pull request as ready for review October 17, 2024 20:45
@dagood dagood requested a review from a team as a code owner October 17, 2024 20:45
Then, add a CI test that runs the following commands in the directory where the script is stored:

```
go install github.com/microsoft/go-infra/install/powershellscript/cmd/microsoftgoinstallscript
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we omit this command? We already explained how to install microsoftgoinstallscript in a previous section, and it should be necessary to reinstall it every time a command is executed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "should" was a "shouldn't" typo, but actually I do think it's relatively necessary. 😄

The microsoftgoinstallscript binary itself contains the script text, so you need to reinstall it whenever you have the wrong version of it installed, and it's not easy to tell, so I think we need to include install in a lot more places than normal to try to make sure people don't stumble on this.

But... maybe the design is too simple, and we need a smarter user-focused command. E.g. a command that doesn't embed the script content and instead looks at go.mod/go.sum and downloads the right version of the script content. (Or maybe the current cmd is ok as a component, but we also need another cmd to wrap it in a friendly way?)

This cmd as it stands is basically an MVP for using a Go dependency to keep a PowerShell script up to date in a remote module, but I see some ways to improve this.

install/powershellscript/README.md Outdated Show resolved Hide resolved
Download the specified version. Supports some aliases. Possible values:
- Latest - the most recent major version.
- Previous - the second most recent major version.
- 2-part version in format go1.A - represents a specific major version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should document whether prerelease versions are supported or not, i.e. 1.23rc1.

qmuntal

This comment was marked as duplicate.

qmuntal

This comment was marked as duplicate.

To update the script, run this in the directory where the script is stored:

```
go get github.com/microsoft/go-infra/install/powershellscript/cmd/microsoftgoinstallscript@latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The package name is too verbose for my old eyes. Users will mainly want to install the CLI rather than using the small Go API in github.com/microsoft/go-infra/install/powershellscript. When the principal "product" is a CLI and not the Go API, it is common to not follow the usual pkgname/cmd/cliname convention but use cliname/pkgname instead. This way the CLI is easier to install (requires less typing!).

Also, the microsoftgoinstallscript cli can potentially be made more generic and install other script types. Even if we don't eventually do it, I don't see a lot of value on putting it under a folder named powershellscript, as we are artificially limiting the scope of the cli.

With all this in mind, this is my suggestion:

  • Rename github.com/microsoft/go-infra/install/powershellscript to github.com/microsoft/go-infra/goinstallscript.
  • Move all files in github.com/microsoft/go-infra/goinstallscript to github.com/microsoft/go-infra/goinstallscript/powershell.
  • Move github.com/microsoft/go-infra/goinstallscript/cmd/microsoftgoinstallscript/main.go to github.com/microsoft/go-infra/goinstallscript.

If we do this, then installing the cli will look like this:

go get github.com/microsoft/go-infra/goinstallscript@latest

qmuntal

This comment was marked as duplicate.

@@ -0,0 +1,59 @@
// Copyright (c) Microsoft Corporation.
Copy link
Contributor

@qmuntal qmuntal Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta question: can we omit the microsoft prefix from the script name and from the new package name? In the future we might want to support downloading the upstream toolchain using this same script and just changing a parameter (which will always default to the Microsoft toolchain). If that happens, then having the Microsoft prefix will feel strange.

I guess the Microsoft prefix is there to avoid conflicting with upstream tooling, but I don't think it will ever conflict. Google want provide a powershell script, and even if it did, users installing both could rename our script with whatever name they like.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

users installing both could rename our script with whatever name they like.

Well, if they use the current util command to keep it up to date, they couldn't. 😄

I think giving it more general branding does sound reasonable. In fact, supporting upstream Go actually could make a lot of sense for this install scenario because it lets people easily run tests on both Microsoft Go and upstream Go in CI and locally.

@@ -0,0 +1,38 @@
# Copyright (c) Microsoft Corporation.
Copy link
Contributor

@qmuntal qmuntal Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta comment: I don't expect that we update the PowerShell script nor the microsoftgoinstallscript package often, maybe one or two times per year, if any. On the other hand, the go-infra is updated quite more often. Also, microsoftgoinstallscript doesn't depend on other go-infra packages.

With this in mind, I would suggest planting a go.mod file in install/powershellscript so that package gets promoted to being it's own module. Then we can version it independently of go-infra. Well, we are currently versioning go-infra, but I think we should definitely version microsoftgoinstallscript.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I've always assumed those still get versioned with the surrounding module, but finally saw https://go.dev/doc/modules/managing-source#multiple-module-source for how that works.

Yeah, we would produce a lot of useless dependabot update PRs for people if we didn't version it separately.

@dagood dagood force-pushed the dev/dagood/pwsh-install-script branch from e0d0cc0 to d74a0d0 Compare October 21, 2024 22:14
@karianna karianna requested a review from qmuntal October 22, 2024 01:11
"github.com/microsoft/go-infra/goinstallscript/powershell"
)

// scriptEmitPackage is a Go package that emits the PowerShell script content. The goinstallscript
Copy link
Contributor

@qmuntal qmuntal Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I don't get the use-case that this indirection tries to tackle. Why someone would want to install multiple versions of the script? How other steps would be necessary to accomplish that?

Also, this new go run approach precludes running goinstallscript in environments without go installed. I think it was a nice feature that before you could plant goinstallscript in a brand new environament and just execute it to install the powershell script. Note that we could support both use-cases by falling back to powershell.Content if exec.LookPath("go") fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants