circleci-cli
circleci-cli copied to clipboard
#422 - Windows Installer
- [x] I have read Contribution Guidelines.
- [x] I have checked for similar issues and haven't found anything relevant.
- [x] This is not a security issue (which should be reported here: https://circleci.com/security/)
See #422 - adds configuration and CI jobs for creating a Windows executable installer for 64-bit installs.
Codecov Report
Merging #426 into master will not change coverage. The diff coverage is
n/a.
@@ Coverage Diff @@
## master #426 +/- ##
=======================================
Coverage 30.86% 30.86%
=======================================
Files 26 26
Lines 3201 3201
=======================================
Hits 988 988
Misses 2119 2119
Partials 94 94
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 423e103...72dba01. Read the comment docs.
Now that tests are passing and the installer is being built nicely, I wonder what the best way of distributing/hosting the installer is for winget, and whether we want to compile and distribute a 32-bit variant as well. @marcomorain Open to suggestions.
This is awesome. I'm downloading the binary now to check out the process.
Some notes (non-blocking):
- The installer package does not have a CircleCI icon
- Windows Defender tries to stop the app from running [1]
- The installer has "Unknown Publisher" in yellow when installing [2]
- The installed app shows an error about
data.ymlwhen launched with no args [3]
1

2

3

Signing the executable might be a bit tricky, since we'll have to obtain a code signing certificate. Unsure the best way to do this, or if it's even worth it given the cost.
- The installer package does not have a CircleCI icon
- Windows Defender tries to stop the app from running [1]
- The installer has "Unknown Publisher" in yellow when installing [2]
- The installed app shows an error about data.yml when launched with no args [3]
I would consider 1 and 4 blocking for this PR, 2 & 3 might require a little more investigation.
Update on this:
- I fixed 4 on Friday
- When I installed the package from the installer
circleci updatefailed to run:
circleci update
You are running 0.1.8068
A new release is available (0.1.7971)
Error: failed to install update: open C:\Program Files\CircleCI\.circleci.exe.new: Access is denied.
I hope this is because we can't write to c:\program files\
@marcomorain I noticed this as well - it's definitely because we don't have write access to that location. Ideally once we have the winget entry this will be a non-issue, and updates will be handled by winget instead, but it does raise an issue as to how we want to handle the update command when installed in this way. We have a similar issue when the snap is installed, although that auto-updates and is less of an issue.
yeah, Snap and Homebrew manage their own updates. Afaik, winget doesn't yet support updating packages, it just allows you to install them.
Handling write permission errors when installed via the CLI is proving a little more difficult than I original imagined. https://stackoverflow.com/a/49148866 is maybe the best option we have, and might help catch other installation environments, but I'm not 100% sold on it.
I was thinking about adding some code here:
https://github.com/CircleCI-Public/circleci-cli/blob/423e103f902f4fb689d7b1f7880636f41fc9551f/update/update.go#L179-L182
to change the error message based on runtime.GOOS. On Windows we could add some language around running the terminal as an administrator.
func helpfulErrorMessage(err error) error {
switch runtime.GOOS {
case "windows":
return errors.Wrap(err, "failed to install the update. Make sure that you choose 'run as administrator' when starting your shell")
default:
errors.Wrap(err, "failed to install update")
}
}
// InstallLatest will execute the updater and replace the current CLI with the latest version available.
func InstallLatest(opts *Options) (string, error) {
release, err := opts.updater.UpdateSelf(opts.Current, opts.slug)
if err != nil {
return "", helpfulErrorMessage(err)
}
return fmt.Sprintf("Updated to %s", release.Version), nil
}
This error message needs to be improved, but this is along the lines that I was thinking.
Looks good to me, I was hoping for something a little more targeted but complexity might get out of hand quickly.
@gmemstr is this still an applicable PR or can it be closed out? If so there are some conflict errors to resolve before we can continue reviewing this. Thank you!
Closing out as no response has been given