Change install context of windows terminal profile to `per-user` #9322

wolimst · 2023-05-30T14:40:27Z

Description

Change installation context of the windows terminal profile to per-user from per-system.

This change is made because installation validation fails in winget ci while executing custom action to replace the installation path of nu.exe and nu.ico in the windows terminal profile file.

Refer discussions in #5812 and microsoft/winget-pkgs#106977

Installation path of the windows terminal profile is changed as below:
- from: C:\ProgramData\Microsoft\Windows Terminal\Fragments\nu\nu.json
- to: C:\Users\<user>\AppData\Local\Microsoft\Windows Terminal\Fragments\nu\nu.json
Custom action to replace the installation path of nu.exe and nu.ico in the json file will be executed without privilege escalation

This change is expected to eliminate the validation failure in winget ci (need to wait until next release to be sure about it), however, it creates some inconsistency in installation context.

The windows terminal profile is installed in per-user context, other files / PATH env variable are installed in per-system context.
Building the installer shows a warning about this: warning LGHT1076 : ICE91: The file 'WindowsTerminalProfileFile' will be installed to the per user directory 'WindowsTerminalProfileAppFolder' that doesn't vary based on ALLUSERS value. This file won't be copied to each user's profile even if a per machine installation is desired.
which means: WT profile will be installed only for the user that executed the installer and it won't be installed for other users in the system. However, the installer is configured to use per-system context, therefore, other files (such as nushell binary nu.exe) will be installed for all users.

It might be better if we provide options for installation context (per-user or per-system) as requested in #5927 in the future, if that doesn't cause problems in winget ci.

User-Facing Changes

Installation path of windows terminal profile will be changed as above.
Windows Terminal profile will be installed only for the user that installed nushell as stated above. The profile should be manually added for other users if needed.

Tests + Formatting

No test is added since this change is related to installation process in Windows and does not contain source code changes.

Following checks are done manually.

Environment

OS: Windows 11 Pro 22H2
Built the installer by referring .github/workflows/release-pkg.nu script

Checks

Install: WT profile should be created in C:\Users\<user>\AppData\Local\Microsoft\Windows Terminal\Fragments\nu\nu.json and it should contain correct path for nu.exe and nu.ico.
- Install (WT profile feature enabled)
- Install (WT profile enabled) -> Manually remove the WT profile file -> Re-run the installer and Repair
Uninstall: WT profile should be removed.
- Install (WT profile enabled) -> Uninstall
- Install (WT profile enabled) -> Re-run the installer and Modify (WT profile disabled)

After Submitting

No relevant documentation to update.

fdncred · 2023-05-30T14:55:06Z

I'm so greatly appreciative of you and your work on this. Thanks so much! I'm hoping this fixes the issue.

It might be better if we provide options for installation context (per-user or per-system) as requested in #5927

I'd be ok with this in the future as long as our winget_pkgs submissions knows to use the -user one and it works.

fdncred · 2023-06-01T13:51:52Z

We're getting close to a release. Let's land this and see if it works.

fdncred · 2023-06-09T22:23:20Z

Welp, it failed but for a different reason. I'm not sure what's going on. It didn't even get submitted to the winget repo.
https://github.com/nushell/nushell/actions/runs/5226501091/jobs/9437222070

wolimst · 2023-06-10T13:29:46Z

I don't have much knowledge about the tool (winget-releaser github action), but looking at the error message and comparing it to the previous job from v0.80, it looks like the number of installer urls is causing the problem.

# v0.81
Executing command: java -jar komac.jar update --id 'Nushell.Nushell' --version 0.81.0 --urls 'https://github.com/nushell/nushell/releases/download/0.81.0/nu-0.81.0-aarch64-pc-windows-msvc.msi,https://github.com/nushell/nushell/releases/download/0.81.0/nu-0.81.0-x86_64-pc-windows-msvc.msi' --submit
  The number of unique installer urls is greater than the number of previous manifest urls

# v0.80
Executing command: java -jar komac.jar update --id 'Nushell.Nushell' --version 0.80.0 --urls 'https://github.com/nushell/nushell/releases/download/0.80.0/nu-0.80.0-x86_64-pc-windows-msvc.msi' --submit

fdncred · 2023-06-10T15:12:30Z

Good catch. I wonder where that's coming from?

fdncred · 2023-06-10T15:15:00Z

I think @sitiom helped with that CI change. Maybe they can help?

fdncred · 2023-06-12T19:04:29Z

The winget_pkgs failed again. Surprise! Here's the latest thread. @wolimst Any thoughts on the two options listed here? microsoft/winget-pkgs#109445 (comment)

wolimst · 2023-06-14T15:16:34Z

@fdncred Sorry it didn't work.

You could add the "requiresElevation" for the machine scope and "prohibitsElevation" for the user scope (as long as no UAC is required).

I think what they are suggesting is related to the winget manifest, not directly related to the installer msi.

From Winget Manifest Schema for installer.yaml:

ElevationRequirement - Indicator for elevation requirements when installing or upgrading packages.
Optional Field

This key represents which scope a package is required to be executed under. Some packages require user level execution while others require administrative level execution.

elevationRequired - Must be run from a shell that is running in an administrative context (e.g - Admin user using powershell/terminal/cmd with "Run as Administrator")
elevationProhibited - Must be run from a shell that is not running in an administrative context
elevatesSelf - If called from a non-administrative context, will request elevation. If called from an administrative context, may or may not request elevation.

installer.yaml manifest for nushell do not contain ElevationRequirement field currently, so I think we should try adding the field as suggested.

About the value for ElevationRequirement, I think we should use elevationRequired for now (until nushell support user context installation), since the binaries such as nu.exe and PATH env variable are installed in system scope, and also Scope: machine is specified in installer.yaml manifest for nushell.

*edit: requiresElevation -> elevationRequired

fdncred · 2023-06-14T15:38:51Z

oh, ok. Is this requiresElevation setting, is that something you can do a PR for us to use next release?

wolimst · 2023-06-14T15:52:55Z

I think we are using the ci tools (Winget Releaser and Komac) for submitting winget PR, is that right?

I don't have much knowledge about those tools, but I think the winget manifest is automatically generated by Komac. If Komac supports ElevationRequirement, then we might update the ci script.
If it is not supported, maybe we can manually create a PR just like the current release (v0.81)? (and hopefully, the field stays on the future releases?)

Best if we can get some help from the contributors who helped with the winget submission ci.

fdncred · 2023-06-14T16:17:35Z

I think we are using the ci tools (Winget Releaser and Komac) for submitting winget PR, is that right?

Yup, I think that's right. It happened here #8070

@sitiom any ideas about this ElevationRequirement?

sitiom · 2023-06-15T00:21:44Z

maybe we can manually create a PR just like the current release

Yes, any changes to the manifest would require a manual PR. Winget Releaser is just used for auto-updating the manifest.

Also, it seems like the installer is failing to install again in a Sandbox?

You might want to resolve this first, as this may be connected to the Winget issue. Related: #8070 (comment)

fdncred · 2023-06-15T11:49:36Z

Also, it seems like the installer is failing to install again in a Sandbox?

Thanks. Good tip about the sandbox but I have no clue how to resolve that at all. Manually rerunning the CI routines to generate a new MSI, like I did in the prior comment, isn't really an option.

We have to figure this out one way or the other.

wolimst · 2023-06-17T10:14:14Z

Thanks for the info! I was able to reproduce the failure in a sandbox. The error message in the installation log is same with that one of the winget moderator found in their ci system.

CAQuietExec:  Error 0xc0000135: Failed in ExecCommon method
CustomAction ReplacePathsInWindowsTerminalProfile returned actual error code 1603 (note this may not be 100% accurate if translation happened inside sandbox)
Action ended 18:49:40: InstallFinalize. Return value 3.

I don't know it for sure, but maybe they are running the ci in a sandbox. So, resolving this might solve the issue we are having in the winget ci. I'll look into this further when I have some free time.

@fdncred In #8070 (comment), it seems like the manually created installer had no install error. Can you share any details when you manually re-created the installer?

fdncred · 2023-06-17T13:14:51Z

Thanks for the info! I was able to reproduce the failure in a sandbox.

That's great news! Yup, 1603 is that same error code.

So, resolving this might solve the issue we are having in the winget ci. I'll look into this further when I have some free time.

Oh, that's awesome news. I hope you can find time and figure out exactly what is going on and how to fix it. That would be a huge win and relief to me!

Can you share any details when you manually re-created the installer?

Sure, but it's a non-trivial manual task, so much so that I tried to document it to match my Windows system here

nushell/.github/workflows/release-pkg.nu

Lines 9 to 40 in 7d301c7

    
           # Instructions for manually creating an MSI for Winget Releases when they fail 
        
           # Added 2022-11-29 when Windows packaging wouldn't work 
        
           # Updated again on 2023-02-23 because msis are still failing validation 
        
           # To run this manual for windows here are the steps I take 
        
           # checkout the release you want to publish 
        
           # 1. git checkout 0.76.0 
        
           # unset CARGO_TARGET_DIR if set (I have to do this in the parent shell to get it to work) 
        
           # 2. $env:CARGO_TARGET_DIR = "" 
        
           # 2. hide-env CARGO_TARGET_DIR 
        
           # 3. let-env TARGET = 'x86_64-pc-windows-msvc' 
        
           # 4. let-env TARGET_RUSTFLAGS = '' 
        
           # 5. let-env GITHUB_WORKSPACE = 'C:\Users\dschroeder\source\repos\forks\nushell' 
        
           # 6. let-env GITHUB_OUTPUT = 'C:\Users\dschroeder\source\repos\forks\nushell\output\out.txt' 
        
           # 7. let-env OS = 'windows-latest' 
        
           # make sure 7z.exe is in your path https://www.7-zip.org/download.html 
        
           # 8. let-env Path = ($env.Path | append 'c:\apps\7-zip') 
        
           # make sure aria2c.exe is in your path https://github.com/aria2/aria2 
        
           # 9. let-env Path = ($env.Path | append 'c:\path\to\aria2c') 
        
           # make sure you have the wixtools installed https://wixtoolset.org/ 
        
           # 10. let-env Path = ($env.Path | append 'C:\Users\dschroeder\AppData\Local\tauri\WixTools') 
        
           # You need to run the release-pkg twice. The first pass, with _EXTRA_ as 'bin', makes the output 
        
           # folder and builds everything. The second pass, that generates the msi file, with _EXTRA_ as 'msi' 
        
           # 11. let-env _EXTRA_ = 'bin' 
        
           # 12. source .github\workflows\release-pkg.nu 
        
           # 13. cd .. 
        
           # 14. let-env _EXTRA_ = 'msi' 
        
           # 15. source .github\workflows\release-pkg.nu 
        
           # After msi is generated, you have to update winget-pkgs repo, you'll need to patch the release 
        
           # by deleting the existing msi and uploading this new msi. Then you'll need to update the hash 
        
           # on the winget-pkgs PR. To generate the hash, run this command 
        
           # 16. open target\wix\nu-0.74.0-x86_64-pc-windows-msvc.msi | hash sha256 
        
           # Then, just take the output and put it in the winget-pkgs PR for the hash on the msi

.
There's no doubt that you'll have to change the paths and such to make it work for you but I tried to list the requirements and exactly how to set all the variables it needs. I usually just end copy copy-n-pasting and running these lines one-by-one.

wolimst · 2023-06-22T16:04:28Z

I think I found the cause of the problem, and after all, it seems like neither windows terminal profile feature nor permission issue are not the main cause. (TLDR is at the bottom of the comment.)

Currently, an installer created by github actions ci for release fails to install in a windows sandbox instance, with 1603 error code when executing custom action for windows terminal profile feature.

I was able to install without an error by changing the installer to ignore the result of the wt profile feature and recreated it from the release ci.

But at this point, executing nu.exe failed with an error The code execution cannot proceed because VCRUNTIME140.dll was not found, which means which means the installer did not contain statically linked MSVC runtime and nu.exe couldn't find MSVC runtime for dynamic linking.

It looks like this error is the cause of the installation failure on wt profile feature, since it executes nushell to update the wt profile configuration.

On the other hand, an installer that is manually created by a user does not have this problem. Looking here and there, I found that nushell is already statically linking the MSVC runtime as below.

nushell/.cargo/config.toml

Lines 1 to 4 in 64319ad

    
           [target.x86_64-pc-windows-msvc] 
        
           # increase the default windows stack size 
        
           # statically link the CRT so users don't have to install it 
        
           rustflags = ["-C", "link-args=-stack:10000000", "-C", "target-feature=+crt-static"]

However, these build configurations are abandoned on the ci, because RUSTFLAGS env variable has the priority which was set by default as RUSTFLAGS="-D warnings" by actions-rust-lang/setup-rust-toolchain. They have documentation about this behavior in their README.

nushell/.github/workflows/release.yml

Lines 73 to 74 in 64319ad

    
               - name: Setup Rust toolchain and cache 
        
                 uses: actions-rust-lang/setup-rust-toolchain@v1.5.0

By removing the RUSTFLAGS env variable, the build configurations above are properly used on the ci, and I was able to install nushell without an error in a windows sandbox instance.

Finally, looking at the history of PRs for winget submission, I found that the verification failure is occurring since v0.73 (winget PR). The wt profile feature was added in v0.65 (release note, winget PR) and had no problem on the verification until v0.73. On the other hand, nushell started using actions-rust-lang/setup-rust-toolchain on the release ci (#7315) since v0.73 (release note). The timing matches to the start of the winget verification error, which backs up my theory about the cause of the winget verification failure.

TLDR:

actions-rust-lang/setup-rust-toolchain on the nushell's release workflow sets RUSTFLAGS="-D warnings" env variable by default, which makes rustflags configuration to statically link MSVC runtime ignored. Therefore, an installer automatically created by the release workflow do not contain statically linked MSVC runtime.
When installing windows terminal profile feature, nushell is used to update paths. However, executing nushell ends up an error because MSVC runtime is not statically linked and there is no pre-installed one in a sandbox instance for dynamic linking.
The timing when the winget verification error started matches with the timing when actions-rust-lang/setup-rust-toolchain was added to the release workflow (i.e. when statically linked MSVC runtime started being excluded from the installer), so it seems like it is the cause of the winget verification failure.

wolimst · 2023-06-22T16:06:27Z

So, I think I'll make a PR to remove RUSTFLAGS env variable on the release ci. If the winget verification failure is indeed connected to the installation failure in a windows sandbox instance described above, then I think it will fix the problem.

Also, if it's ok, I would like to revert the changes made by this PR (that changed install context of wt profile to per-user) back to per-system, since the installer is based on per-system installation and having different installation context among the features creates unnecessary inconsistency.

@fdncred Does this sound good to you?

fdncred · 2023-06-22T18:31:09Z

First of all, wow, big wow! I'm so impressed and thankful that you were able to figure out exactly what is going on. Thank you so much!

Does this sound good to you?

Yes, it does. I'm excited to see it work.

wolimst · 2023-06-23T18:16:56Z

Thanks. Happy to help! I just created two PRs discussed in the previous comments. I'm hopeful that it will finally resolve the verification failure in the winget PRs since it looks quite promising, but let's wait for the next release to see if it works..!

…9513) # Description Prevent `rustflags` build configuration from being ignored in github actions ci workflows. [actions-rust-lang/setup-rust-toolchain](https://github.com/actions-rust-lang/setup-rust-toolchain/) used in ci workflows sets `RUSTFLAGS="-D warnings"` env variable by default. It has priority over some other sources of `rustflags` as described in [the cargo reference](https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags). Nushell is using `target.x86_64-pc-windows-msvc.rustflags` to statically link MSVC runtime in windows platform, but this config is being ignored in ci workflows. By unsetting `RUSTFLAGS` env variable, `rustflags` build configurations will be properly used in ci workflows. I assume that this was the cause of the installer verification failures on the recent winget submission PRs. For more details, refer discussions in microsoft/winget-pkgs#106977 and #9322 (comment). # User-Facing Changes Pre-built releases for windows that are created by the release ci workflow will now contain statically linked MSVC runtime. # Tests + Formatting Confirmed successful installation in a windows sandbox instance, which was failing before. It also contains changes made in #9514. Release ci workflow: https://github.com/wolimst/nushell/actions/runs/5357440849 Installer: https://github.com/wolimst/nushell/releases/tag/0.81.0-test # After Submitting Need to check the installer verification result in a winget submission PR for the next release, if this PR gets merged.

# Description Revert installation context of windows terminal profile to per-system, since installation context of other features are per-system, and having different installation context creates unnecessary inconsistency. Installation context change of windows terminal profile to per-user (#9322) was made to resolve the recent installer verification failure on winget submission PRs, but the cause of this problem seems to be in another place (#9513). For more details, refer discussions in #5812 and #9322. # User-Facing Changes Windows terminal profile for nushell will be installed in system context. Installation path will be changed as below: - before: `C:\Users\<user>\AppData\Local\Microsoft\Windows Terminal\Fragments\nu\nu.json` - to: `C:\ProgramData\Microsoft\Windows Terminal\Fragments\nu\nu.json` # Tests + Formatting Confirmed successful installation of nushell and windows terminal profile file in windows using below installer that created by release ci workflow. It also contains changes made in #9513. Release ci workflow: https://github.com/wolimst/nushell/actions/runs/5357440849 Installer: https://github.com/wolimst/nushell/releases/tag/0.81.0-test

sitiom · 2023-06-28T01:42:45Z

Seems like it's fixed now 🎉: microsoft/winget-pkgs#110786

fdncred · 2023-06-28T10:37:55Z

great work here! our winget create pr action failed for some reason, but after the winget pr got created, it was totally successful! i'm just not sure how the pr got created.

fdncred · 2023-10-18T01:44:31Z

Welp, winget is broke again. I just went through and all the target_rustflags: '' seem to be set. No idea what's going on now. Thinking of removing the windows terminal fragment maybe.

wolimst · 2023-10-18T12:00:05Z

Welp, winget is broke again. I just went through and all the target_rustflags: '' seem to be set. No idea what's going on now. Thinking of removing the windows terminal fragment maybe.

Added comment in #5812 (comment).

fdncred · 2023-10-18T12:05:06Z

Thanks @wolimst. You can see here where i checked for target_rustflags: ''. apparently that was the wrong thing to check for 🤣

fdncred · 2024-05-01T13:27:52Z

@wolimst Any time to help diagnose our latest winget + sandbox failures? I have two suspicions:

Same old one, UAC is required to write to system level folder, thinking of switching to user level folder. It would be nice to have per-user and per-system choices, defaulting to per-user I think. Not sure how having choices would affect wingets auto-install validation.
I'm not convinced our guid is calculated correctly in our nu.json WT fragment file as per https://learn.microsoft.com/en-us/windows/terminal/json-fragment-extensions.
I've checked the with rustflags and I think they're all still set right.

Any help here would be greatly appreciated.
Latest winget PR with failures microsoft/winget-pkgs#151729

I think this is what the guid should be 47302f9c-1ac4-566c-aa3e-8cf29889d6ab and how I generated it.

❯ python
Python 3.10.11 (tags/v3.10.11:7d4cc5a, Apr  5 2023, 00:38:17) [MSC v.1929 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import uuid
>>> terminalNamespaceGUID = uuid.UUID("{f65ddb7e-706b-4499-8a50-40313caf510a}")
>>> appNamespaceGUID = uuid.uuid5(terminalNamespaceGUID, "Nushell".encode("UTF-16LE").decode("ASCII"))
>>> profileGUID = uuid.uuid5(appNamespaceGUID, "nu".encode("UTF-16LE").decode("ASCII"))
>>> print(f"{{{profileGUID}}}")
{47302f9c-1ac4-566c-aa3e-8cf29889d6ab}
>>>

wolimst added 2 commits May 30, 2023 21:53

Change install context of windows terminal profile to per-user

c2cba2a

Replace single quotes to backticks in custom action command

52e49df

amtoine added platform-specific platform-specific issues windows A Windows specific issue labels May 30, 2023

fdncred merged commit e48b949 into nushell:main Jun 1, 2023
16 checks passed

fdncred mentioned this pull request Jun 1, 2023

revert pr 5812, adding windows terminal fragment #9221

Closed

fdncred mentioned this pull request Jun 10, 2023

use winget releaser action for manifest submission #8070

Merged

fdncred mentioned this pull request Jun 10, 2023

Add aarch64-pc-windows-msvc building target for nu binaries #9162

Merged

This was referenced Jun 23, 2023

Prevent rustflags build config from being ignored in ci workflows #9513

Merged

Revert install context of windows terminal profile to per-system #9514

Merged

fdncred mentioned this pull request Jun 28, 2023

New version: Nushell.Nushell version 0.82.0 microsoft/winget-pkgs#110786

Merged

wolimst mentioned this pull request Oct 18, 2023

Add Windows Terminal profile and icon in Windows control panel #5812

Merged

13 tasks

starthal mentioned this pull request Dec 5, 2023

winget package: Reduce scope to "per-user" #11240

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change install context of windows terminal profile to `per-user` #9322

Change install context of windows terminal profile to `per-user` #9322

wolimst commented May 30, 2023

fdncred commented May 30, 2023

fdncred commented Jun 1, 2023

fdncred commented Jun 9, 2023

wolimst commented Jun 10, 2023

fdncred commented Jun 10, 2023

fdncred commented Jun 10, 2023

fdncred commented Jun 12, 2023

wolimst commented Jun 14, 2023 •

edited

fdncred commented Jun 14, 2023

wolimst commented Jun 14, 2023 •

edited

fdncred commented Jun 14, 2023

sitiom commented Jun 15, 2023

fdncred commented Jun 15, 2023

wolimst commented Jun 17, 2023 •

edited

fdncred commented Jun 17, 2023

wolimst commented Jun 22, 2023

wolimst commented Jun 22, 2023 •

edited

fdncred commented Jun 22, 2023

wolimst commented Jun 23, 2023 •

edited

sitiom commented Jun 28, 2023 •

edited

fdncred commented Jun 28, 2023

fdncred commented Oct 18, 2023

wolimst commented Oct 18, 2023 •

edited

fdncred commented Oct 18, 2023

fdncred commented May 1, 2024 •

edited

Change install context of windows terminal profile to per-user #9322

Change install context of windows terminal profile to per-user #9322

Conversation

wolimst commented May 30, 2023

Description

User-Facing Changes

Tests + Formatting

Environment

Checks

After Submitting

fdncred commented May 30, 2023

fdncred commented Jun 1, 2023

fdncred commented Jun 9, 2023

wolimst commented Jun 10, 2023

fdncred commented Jun 10, 2023

fdncred commented Jun 10, 2023

fdncred commented Jun 12, 2023

wolimst commented Jun 14, 2023 • edited

fdncred commented Jun 14, 2023

wolimst commented Jun 14, 2023 • edited

fdncred commented Jun 14, 2023

sitiom commented Jun 15, 2023

fdncred commented Jun 15, 2023

wolimst commented Jun 17, 2023 • edited

fdncred commented Jun 17, 2023

wolimst commented Jun 22, 2023

wolimst commented Jun 22, 2023 • edited

fdncred commented Jun 22, 2023

wolimst commented Jun 23, 2023 • edited

sitiom commented Jun 28, 2023 • edited

fdncred commented Jun 28, 2023

fdncred commented Oct 18, 2023

wolimst commented Oct 18, 2023 • edited

fdncred commented Oct 18, 2023

fdncred commented May 1, 2024 • edited

Change install context of windows terminal profile to `per-user` #9322

Change install context of windows terminal profile to `per-user` #9322

wolimst commented Jun 14, 2023 •

edited

wolimst commented Jun 14, 2023 •

edited

wolimst commented Jun 17, 2023 •

edited

wolimst commented Jun 22, 2023 •

edited

wolimst commented Jun 23, 2023 •

edited

sitiom commented Jun 28, 2023 •

edited

wolimst commented Oct 18, 2023 •

edited

fdncred commented May 1, 2024 •

edited