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

fix: Do not pack "elevate.exe" when not needed #1621

Closed

Conversation

MariaDima
Copy link
Contributor

Close #1620

@develar
Copy link
Member

develar commented Jun 6, 2017

Discussion in the issue. PR cannot be accepted in this form for now.

@develar
Copy link
Member

develar commented Jun 8, 2017

I am sorry for misunderstanding — elevate.exe is not required for NSIS. It is required only for updater. You can simply add option like doNotPackElevateHelper. Your second commit is not required. First (7e9cdb9) is enough — you only need to add and respect doNotPackElevateHelper.

@MariaDima
Copy link
Contributor Author

Yes! The updater, on error, uses the executable to rerun the installer!

I'll revert the second commit and add the new "doNotPackElevateHelper" option.

One question:
Can we do the same for the signtool?
On my second commit I generalized it the removed all files that do not relate to the application and are not needed after installation.
If this is ok, maybe I can call the new option "doNotPackHelperFiles"

@develar
Copy link
Member

develar commented Jun 8, 2017

Can we do the same for the signtool?

For some reasons I decided that powershell is not suitable to verify code signature. But after your PR it seems it is not required anymore. We can simply do not pack and remove this line.

I prefer separate options.

@MariaDima
Copy link
Contributor Author

Ok, thanks!
I'll updated as following:

  • Revert to the first commit
  • Define "doNotPackElevateHelper" under Nsis options
  • Use the new option instead of the existing "allowElevation"
  • Do not pack the signTool.exe at all

Please correct me, if I misunderstood.

@develar
Copy link
Member

develar commented Jun 8, 2017

Yes. 👍

@MariaDima
Copy link
Contributor Author

@develar are you aware of the test failures?
I see it's a general issue, not related to the changes of this patchset.
Are you working on this? Can I help here?

@develar
Copy link
Member

develar commented Jun 8, 2017

@MariaDima Do not worry, all is ok and PR will be merged in minutes.

@develar develar closed this in 69c3614 Jun 8, 2017
@develar
Copy link
Member

develar commented Jun 8, 2017

Thanks for PR!

@MariaDima
Copy link
Contributor Author

Thanks!

@MariaDima
Copy link
Contributor Author

@develar we forgot to mention "packElevateHelper" in the wiki. Should I add it or you prefer to not expose this option?

@develar
Copy link
Member

develar commented Jun 9, 2017

@MariaDima Wiki will be synced a little bit later, because 18.7.0 not yet latest release, but pre-release.

@MariaDima
Copy link
Contributor Author

Perfect, thanks!

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.

2 participants