Skip to content

Windows installer and app icon.[CPP-324] #208

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Nov 10, 2021
Merged

Windows installer and app icon.[CPP-324] #208

merged 8 commits into from
Nov 10, 2021

Conversation

john-michaelburke
Copy link
Collaborator

@john-michaelburke john-michaelburke commented Nov 6, 2021 •
edited
Loading

  • Nsis installer script (adapted from fbs implementation).
  • Added app icon while running the application.
  • Added app icon to the exe and shortcuts for windows.

Screen Shot 2021-11-05 at 6 12 56 PM

Screen Shot 2021-11-05 at 6 11 51 PM

@john-michaelburke john-michaelburke changed the title Windows installer.[CPP-324] Windows installer and app icon.[CPP-324] Nov 6, 2021
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/installers branch 6 times, most recently from 3be1bd5 to d838342 Compare November 6, 2021 06:03
@john-michaelburke
Copy link
Collaborator Author

Until I finish up the remaining installers, I will just have the installers be uploaded as artifacts on a tag. Here is the windows artifact from a test release:
https://github.com/swift-nav/console_pp/actions/runs/1428470537

@john-michaelburke john-michaelburke marked this pull request as ready for review November 6, 2021 06:38
@john-michaelburke john-michaelburke requested a review from a team November 6, 2021 06:39
silverjam
silverjam reviewed Nov 7, 2021
View reviewed changes
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/installers branch from d411686 to 1da03fc Compare November 8, 2021 18:00
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/installers branch from 1da03fc to 99a798d Compare November 8, 2021 21:18
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/installers branch from 99a798d to 2839404 Compare November 8, 2021 21:27
@silverjam
Copy link
Contributor

Nice, is there a test build of this somewhere?

@john-michaelburke
Copy link
Collaborator Author

Nice, is there a test build of this somewhere?

I just kicked off tag now. When it finishes itll be in the artifacts under Windows-installer:
https://github.com/swift-nav/console_pp/actions/runs/1437440398

I'll shoot you a ping when it is done.

@john-michaelburke
Copy link
Collaborator Author

Here it is @silverjam :
https://github.com/swift-nav/console_pp/suites/4295444928/artifacts/112032317

silverjam
silverjam suggested changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

This installer throws up a UAC (user access control) prompt when running in order to grab administrative privileges, which should not be required when installing into %localappdata%.

There are ways to arrange for NSIS to do a per-user installer, see:

This allows installation to be configured as machine-wide or single-user:

However... in the interest of time I think we should just revert back to using C:\Program Files\Swift Navigation\Swift Navigation Console.

Let's log a new issue to address the per-user installer as a new feature.

@john-michaelburke
Copy link
Collaborator Author

This installer throws up a UAC (user access control) prompt when running in order to grab administrative privileges, which should not be required when installing into %localappdata%.

There are ways to arrange for NSIS to do a per-user installer, see:

This allows installation to be configured as machine-wide or single-user:

However... in the interest of time I think we should just revert back to using C:\Program Files\Swift Navigation\Swift Navigation Console.

Let's log a new issue to address the per-user installer as a new feature.

I found a one liner that seems to have removed the UAC prompt.

@john-michaelburke
Copy link
Collaborator Author

I have added a CACHE_SECRET to our caching github actions steps. Reading through some suggestions on SO, it seems using this is an easier way to wipe your caches as github does not currently have a native way of doing this. Simply update the secret to a different value and rerun your workflow.

@john-michaelburke john-michaelburke force-pushed the john-michaelburke/installers branch from 7cbd99b to 01fc369 Compare November 9, 2021 19:16
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/installers branch from 01fc369 to 383b650 Compare November 9, 2021 19:25
@silverjam
Copy link
Contributor

@john-michaelburke can you push (or re-push) a tag to generate a new installer?

@john-michaelburke
Copy link
Collaborator Author

@silverjam https://github.com/swift-nav/swift-toolbox/suites/4305587806/artifacts/112446320

silverjam
silverjam suggested changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

This almost works, not seeing the "Unisntall" button work in the Add/Remove programs UI:

image

Manually running the Uninstall.exe works though

installers/Windows/Installer.nsi Outdated
;--------------------------------
;Pages

!define MUI_WELCOMEPAGE_TITLE "version ${VERSION}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's define a VERSION3 variable for display here (with 3 components) and VERSION4 (same as the current VERSION variable) for use in VIProductVersion.

Then:

Suggested change
!define MUI_WELCOMEPAGE_TITLE "version ${VERSION}"
!define MUI_WELCOMEPAGE_TITLE "Version ${VERSION3}"

@john-michaelburke john-michaelburke force-pushed the john-michaelburke/installers branch 2 times, most recently from 87cd8d8 to e6271bf Compare November 10, 2021 04:12
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/installers branch from e6271bf to 38c2e37 Compare November 10, 2021 04:26
@silverjam
Copy link
Contributor

@john-michaelburke windows github action runner was borked, apparently a console.exe process was stuck "running" even though no UI was showing

@john-michaelburke
Copy link
Collaborator Author

@silverjam The installer looks good on my end:
https://github.com/swift-nav/swift-toolbox/suites/4308952915/artifacts/112854547

silverjam
silverjam reviewed Nov 10, 2021
View reviewed changes
installers/Windows/Installer.nsi
DeleteRegKey SHCTX "${UNINST_KEY}"
FunctionEnd
!macroend
!insertmacro Uninstall ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need both of these macro calls?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah when using a call inside of the uninstaller the function/macro must be prefixed by "un." so this allows us to call the same function before installing and when uninstalling.

@silverjam
Copy link
Contributor

@silverjam The installer looks good on my end:

https://github.com/swift-nav/swift-toolbox/suites/4308952915/artifacts/112854547

Thanks, I'll give it a look in a bit, is Stefan waiting on this?

@john-michaelburke john-michaelburke merged commit b9b33d8 into main Nov 10, 2021
@john-michaelburke john-michaelburke deleted the john-michaelburke/installers branch November 10, 2021 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@silverjam silverjam silverjam approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@john-michaelburke @silverjam