Add Windows Terminal profile and icon in Windows control panel #5812
Conversation
|
Awesome, thank you for the contribution! It will be really nice to finally have this feature, and I really appreciate the detailed PR description + manual testing. |
|
You're welcome! Though, there is one problem (this is why the PR is in draft) Windows Terminal shows empty icon for the installed I think maybe there's some kind of problem in the icon, Maybe I can re-generate |
Sounds good, that's worth a try. |
|
@wolimst I just wanted to jump in and say, nice work on this PR! it's a pleasure to read such detailed notes in the PR body. Good job! |
Procedure: opened the original file with GIMP and simply overwrited it
|
I used the original Simply opened and overwrited the file with GIMP. Maybe the problem was related to metadata..? Changed PR to ready for review, since the recreated icon solved the issue. |
|
The icon is shown somewhat not clearly though, because the high-res image is resized. I think this might be resolved by including multiple images with various resolution (with larger nu letter in low-res image?) in the |
|
That's a good idea; we might also need to pick a logo that looks good even at small resolutions. We're not 100% set on the current one. I think this is good to merge now and we can tweak the icon later; thanks again, this is a great first PR! |
Looking forward to it!
Thanks for the merge, and thanks for having me to contribute! |
…ll#5812) * Show icon in Windows 'Add/Remove Programs' control panel * Add install option for Windows Terminal profile * Re-create icon because the icon was not shwon in Windows Terminal Procedure: opened the original file with GIMP and simply overwrited it
|
@wolimst This has been causing the winget ci to fail for months. The winget folks finally narrowed it down to this error. Which seems to jive with these changes in the wsx file Lines 375 to 393 in 057de06 Any ideas how to keep this from failing? I'm just guessing but we've changed the syntax for the update command recently. I wonder if that's what is causing it to puke? UPDATE: I can't get this to work, which I think is what it's running. |
|
@fdncred Running the msi installer built from the source / Installing nu with winget created the windows terminal profile without an error, and I couldn't reproduce the problem. Here is the nushell command to replace the paths, ran by the installer. Notice single quotes are used inside the double quotes. Relevant part in the msi installer log, which includes the command (you might need to scroll to the right to see it) So, I think the problem is in the winget ci maybe?
Looking for the error code 1603 from the winget ci, I found following information about the causes.
Just guessing, but as described in the last item of the possible causes above, I think maybe it is a permission issue in the ci. Currently, the path replacing command is ran with system privileges, but I think the ci system wouldn't expose full privileges for custom script for obvious reasons. So, maybe we can change the installer to add windows terminal profile for the current user only, without elevated privileges, and see how it goes with the ci. Do you know whether we can run the winget ci for testing purpose? |
|
thanks for the great research @wolimst. i appreciate all your work. i installed the msi on my win 11 laptop and never saw that nu.json fragment, nor could i get windows terminal to recognize it. so, it's odd to me that it's working for you. using a path that doesn't require full privileges sounds like something that should be tried. i'm not sure about the winget ci, maybe if you fork the winget ci repo and run the ci locally? |
That is odd.. I have looked into winget ci repo, but I think the validation is done by calling azure functions (something like aws lambda I guess?), and I couldn't find any ci scripts or tools for us to run the validation ci locally. Some references:
So, what is our next approach? I think I can change the install script related to wt profile to use user context instaed of system context, and we can see how it goes with the ci on the next release.
But this will reduce some consistency on the context of the installation: executables and PATH env variable will be installed for all users, while the wt profile fragment will be installed for the current user only. What are your thoughts? |
|
It's definitely not in this path on my windows 11 machine (C:\ProgramData\Microsoft\Windows Terminal\Fragments\nu\nu.json). I tried putting it there and there were permission issues that I had to override and even when I got everything in place with the correct paths in the json, windows terminal didn't recognize it. No clue what's going on. TBH, I thought i was going where I have other fragments which is \users<username>\appdata\local\microsoft\windows terminal\fragments\nu\nu.json. I tried putting it in there but i can't get that to be recognized by WT as well. I'm sure it's me. Is there some WT documentation that says where these fragment files can/should be located? I think the next step should be putting the fragment in a path that the user has permissions to. |
You can find it in this document, but it says only those two paths,
Sure, I'll try to create a PR for this soon. |
# 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`. - [x] Install (WT profile feature enabled) - [x] Install (WT profile enabled) -> Manually remove the WT profile file -> Re-run the installer and Repair - Uninstall: WT profile should be removed. - [x] Install (WT profile enabled) -> Uninstall - [x] Install (WT profile enabled) -> Re-run the installer and Modify (WT profile disabled) # After Submitting No relevant documentation to update.
# 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
|
@wolimst Winget failed again in release 0.86.0. Here's the log file entries. Any ideas why I'm guessing that the problem is that it's installing here I think this is part of the problem. Looking in the sandbox, there is no I'm not sure. Here's a log I created trying to manually install in the sandbox. full log |
|
@fdncred Hello again!
Didn't have time to reproduce it on my end, but I don't think that the root cause is Quote from #9513, brief summary of the cause:
The error code is same as the last time Seems like the changes made by #9513 to preserve nushell/.github/workflows/release.yml Lines 81 to 83 in 5d8763e nushell/.github/workflows/release.yml Lines 169 to 171 in 5d8763e Maybe re-add the removed option |
|
dang it, i checked that yaml and thought i saw the rust flags there. i'll take a closer look. it's so good having a second set of eyes on this stuff. thanks @wolimst!! |
|
Oh, sorry, that's my bad, I have never notice that, We should Add some comments there to prevent recurrence |
|
Yes, I also think that we need some comments about it, because it is really easy to overlook, and also it is quite hard to find where the cause is, from the problems caused by it. More detail for the |
|
Comments are added here https://github.com/nushell/nushell/pull/10757/files Winget is cooking with these latest changes/compilations now. microsoft/winget-pkgs#122956 |
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> Fix winget release submission error, more details could be found here: #5812 (comment) @fdncred @wolimst # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --> --------- Co-authored-by: Darren Schroeder <[email protected]>
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> Fix winget release submission error, more details could be found here: nushell#5812 (comment) @fdncred @wolimst # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --> --------- Co-authored-by: Darren Schroeder <[email protected]>
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> Fix winget release submission error, more details could be found here: nushell#5812 (comment) @fdncred @wolimst # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --> --------- Co-authored-by: Darren Schroeder <[email protected]>
Description
Windows Installer generation script (
wix/main.wxs) is updated to support following features:install_dir/nu.ico, to show icon in Windows TerminalChecks
Since the changes are only related to installation process in Windows, and do not contain source code changes, no test is added. Instead, I have checked following things.
.github/workflows/release-pkg.nuscript.github/workflows/release.yml)Checks for Windows Terminal profile
%PROGRAMDATA%/Microsoft/Windows Terminal/Fragments/nu/nu.json) and the file should contain correct path for the nushell executable and the iconChecks for icon in control panel
Tests
Make sure you've done the following:
-> Not applicable for the changes
-> See
Checkssection aboveMake sure you've run and fixed any issues with these commands:
cargo fmt --all -- --checkto check standard code formatting (cargo fmt --allapplies these changes)cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collectto check that you're using the standard code stylecargo test --workspace --features=extrato check that all the tests pass