Fix shortcut script when installed with UAC on #6099

gpduck · 2018-02-03T23:19:27Z

PR Summary

Moves the working directory script to execute during the elevated portion of the install process to prevent access denied errors during install.

Fixes #6095

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
[NA] User facing Documentation needed
- [NA] Issue filed - Issue link:
Change is not breaking
[NA] Make sure you've added a new test if existing tests do not effectively test the code changed
- [NA] Add [feature] if the change is significant or affects feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

iSazonov · 2018-02-04T12:39:01Z

assets/Product.wxs

-    <CustomAction Id="SaveShortcutPath" Property="ShortcutPath" Value="[$ApplicationProgramsMenuShortcut]$(var.ProductSemanticVersionWithNameAndOptionalArchitecture).lnk" />
-    <CustomAction Id="FixShortcutWorkingDirectory" Script="vbscript" HideTarget="no" Impersonate="no">
-      shortcutPath = Session.Property("ShortcutPath")
+    <CustomAction Id="SaveShortcutPath" Property="FixShortcutWorkingDirectory" Value="[$ApplicationProgramsMenuShortcut]$(var.ProductSemanticVersionWithNameAndOptionalArchitecture).lnk" />


Do we really need rename ShortcutPath to FixShortcutWorkingDirectory?

The property name must match the ID of the custom action in order to populate CustomActionData.

https://stackoverflow.com/questions/11233267/how-to-pass-customactiondata-to-a-customaction-using-wix

Thanks for clarify! It is not obvious. Please add the comment with the link.

I know that WiX is not self-explanatory or intuitive to learn but do we really need to document basic WiX stuff here?

Somebody can change the name. We haven't msi package tests. So I'd prefer a protective comment.

Also I'd use SetShortcutWorkingDirectory name.

SteveL-MSFT · 2018-02-05T01:39:12Z

cc @bergmeister

bergmeister · 2018-02-05T12:50:48Z

I have tested this works when run via GUI or quiet install. However, I have some comments about the code:

The custom actions could also have a Return="check" attribute that inspects the exit code and would make the installer fail in case there is a problem. I will leave this decision up to you @SteveL-MSFT

bergmeister · 2018-02-05T12:55:04Z

assets/Product.wxs

-      <Custom Action="SaveShortcutPath" After="InstallFinalize" />
-      <Custom Action="FixShortcutWorkingDirectory" After="SaveShortcutPath">
+      <Custom Action="SaveShortcutPath" Before="FixShortcutWorkingDirectory" />
+      <Custom Action="FixShortcutWorkingDirectory" Before="InstallFinalize">


I find this style hard to read/maintain. It should be OK to run it e.g. just after InstallFiles and would make it more readable:

<Custom Action="SaveShortcutPath" After="InstallFiles" /> <Custom Action="FixShortcutWorkingDirectory" After="SaveShortcutPath"> ``` This is just personal preference, so feel free to interpret it how you like (i.e. this is not a 'you must do')

SteveL-MSFT · 2018-02-09T22:32:26Z

@bergmeister agree that we should leverage the exit code check and fail the install

TravisEz13 · 2018-02-09T23:36:23Z

I think the exit code check should be a separate issue. There are many things to consider, such as what is Windows Script Host is disabled

TravisEz13

Please use SetShortcutWorkingDirectory for the name of the script custom action

iSazonov · 2018-02-10T11:38:01Z

Before the change MSI package fail but not revert (leave already copied files).

TravisEz13 · 2018-02-12T20:32:31Z

@iSazonov

Before the change MSI package fail but not revert (leave already copied files).

Are you saying after this change, if the script fails the package fails?

If so, perhaps we should revert this. Scripts can be unreliable and making it a critical part of the package doesn't seem like a good idea.

iSazonov · 2018-02-13T06:21:49Z

We cannot predict what will cause the installer to crash. We can only confidently say that our installer will be enhanced in the future and such crash reasons will become more.

In any case, we need to think about implementing rollback action. Sample https://medium.com/@thehenrytsai/uninstall-vs-rollback-custom-action-in-wix-beed702d45c1

TravisEz13 · 2018-02-13T18:00:51Z

We can predict it. Script actions have a high historical failure rate due to factors unrelated to the script

gpduck · 2018-02-13T19:42:21Z

So based on my research, the options are:

WSH based script
Custom action as C# plugin wrapping COM components for editing shortcuts
Manually create shortcut (which would probably be a custom plugin)

I'm open to other suggestions as well?

bergmeister · 2018-02-13T20:18:04Z

First of all, I think we should revert commit 795c61125a531d62784e299a3159844b733e8a91 in the meantime (that's what source control is for), especially given the upcoming pre-release next week on 2/22.
My opinion on error handling (and I came up to this conclusion after thinking of the similar problem that I have with Enable-PSRemoting in my current PR 5999)

If the successful execution of an installer script is critical and if the failure messes up the installation, then the error code must be respected and the installation should fail (and also rollback the custom action if required)
If the execution of an installer script is optional and a failure has no effect on the installation apart from the fact that the optional feature could not be installed, then not returning the exit code and continuing the install makes sense. An informal message is nice to have but hard to achieve because it requires stuff like e.g. C# based custom actions.
Let me know what you think.

About the specific problem: although it's hard to bootstrap custom C# actions the first time, I think they would be very welcome and helpful for future installer features (like e.g. PR 5999)

iSazonov · 2018-02-14T04:39:21Z

I don't think plugin custom actions are more reliable than scripts. For example, if we lock a system using a GPO or third-party security product, we will get the same installation error. However, if we want an advanced installer, then definitely we should stick to one way of creating extensions and handling errors and I believe it will be C#. In this case, we must also have the installation tests.

I think that first we need to define our strategy.
If we are very conservative, it will require minimal effort.
If we want to make the installer very advanced, it will require a lot of development and support costs. I'm not sure this is urgently needed right now. I would postpone this until the transition to .Net Core 2.1 version that might require important changes to our installation packages for all platforms.

TravisEz13 · 2018-02-14T17:34:40Z

@bergmeister @iSazonov
I agree with @bergmeister about

reverting the previous commit
the general description of how robust the custom action will be.
that optional custom actions should be implemented as a compiled custom action
a C# based custom action infrastructure would be good to have (as long as it can be dotnet core based)

bergmeister · 2018-02-20T22:05:48Z

@TravisEz13 Commit 795c61125a531d62784e299a3159844b733e8a91 is still not reverted. Is someone stuck inside vim? 😜
https://stackoverflow.blog/2017/05/23/stack-overflow-helping-one-million-developers-exit-vim/

TravisEz13 · 2018-02-20T23:24:43Z

#6203 has been submitted

iSazonov · 2018-02-24T05:52:12Z

@gpduck Please resolve merge conflicts.

stale · 2018-03-26T06:37:04Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. Thank you for your contributions.

TravisEz13 · 2018-03-26T19:16:12Z

I think we can close this for now. See #6099 (comment)

Fix shortcut script when installed with UAC on

d5cb7e0

iSazonov reviewed Feb 4, 2018

View reviewed changes

iSazonov self-assigned this Feb 4, 2018

iSazonov requested review from SteveL-MSFT and TravisEz13 February 4, 2018 12:42

bergmeister reviewed Feb 5, 2018

View reviewed changes

bergmeister mentioned this pull request Feb 5, 2018

Msi installer: smoke test #6105

Merged

9 tasks

TravisEz13 requested changes Feb 9, 2018

View reviewed changes

stale bot added the Stale label Mar 26, 2018

TravisEz13 closed this Mar 26, 2018

Fix shortcut script when installed with UAC on #6099

Fix shortcut script when installed with UAC on #6099

Uh oh!

Conversation

gpduck commented Feb 3, 2018 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

iSazonov Feb 4, 2018

Choose a reason for hiding this comment

Uh oh!

gpduck Feb 4, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

bergmeister Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Feb 5, 2018

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Feb 5, 2018

Uh oh!

bergmeister commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergmeister Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Feb 9, 2018

Uh oh!

TravisEz13 commented Feb 9, 2018

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Feb 10, 2018

Uh oh!

TravisEz13 commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Feb 13, 2018

Uh oh!

TravisEz13 commented Feb 13, 2018

Uh oh!

gpduck commented Feb 13, 2018

Uh oh!

bergmeister commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Feb 14, 2018

Uh oh!

TravisEz13 commented Feb 14, 2018

Uh oh!

bergmeister commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 commented Feb 20, 2018

Uh oh!

iSazonov commented Feb 24, 2018

Uh oh!

stale bot commented Mar 26, 2018

Uh oh!

TravisEz13 commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gpduck commented Feb 3, 2018 •

edited by iSazonov

Loading

bergmeister commented Feb 5, 2018 •

edited

Loading

bergmeister Feb 5, 2018 •

edited

Loading

TravisEz13 commented Feb 12, 2018 •

edited

Loading

bergmeister commented Feb 13, 2018 •

edited

Loading

bergmeister commented Feb 20, 2018 •

edited

Loading