circleci-cli icon indicating copy to clipboard operation
circleci-cli copied to clipboard

#422 - Windows Installer

Open gmemstr opened this issue 5 years ago • 12 comments

  • [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.

gmemstr avatar Jun 05 '20 11:06 gmemstr

Codecov Report

Merging #426 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           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 data Powered by Codecov. Last update 423e103...72dba01. Read the comment docs.

codecov[bot] avatar Jun 05 '20 14:06 codecov[bot]

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.

gmemstr avatar Jun 05 '20 15:06 gmemstr

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.yml when launched with no args [3]

1

image

2

image

3

image

marcomorain avatar Jun 05 '20 19:06 marcomorain

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.

  1. The installer package does not have a CircleCI icon
  2. Windows Defender tries to stop the app from running [1]
  3. The installer has "Unknown Publisher" in yellow when installing [2]
  4. 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.

gmemstr avatar Jun 08 '20 07:06 gmemstr

Update on this:

  • I fixed 4 on Friday
  • When I installed the package from the installer circleci update failed 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 avatar Jun 08 '20 10:06 marcomorain

@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.

gmemstr avatar Jun 08 '20 10:06 gmemstr

yeah, Snap and Homebrew manage their own updates. Afaik, winget doesn't yet support updating packages, it just allows you to install them.

marcomorain avatar Jun 08 '20 10:06 marcomorain

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.

gmemstr avatar Jun 08 '20 11:06 gmemstr

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.

marcomorain avatar Jun 08 '20 12:06 marcomorain

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.

marcomorain avatar Jun 08 '20 12:06 marcomorain

Looks good to me, I was hoping for something a little more targeted but complexity might get out of hand quickly.

gmemstr avatar Jun 08 '20 12:06 gmemstr

@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!

corinnesollows avatar Aug 24 '22 18:08 corinnesollows

Closing out as no response has been given

corinnesollows avatar Sep 28 '22 18:09 corinnesollows