Skip to content

Conversation

@hustcer
Copy link
Contributor

@hustcer hustcer commented May 12, 2025

feat: Add MSI install tests for winget and msiexec, Related PR: nushell/nushell#15690

@nushell nushell deleted a comment from github-actions bot May 12, 2025
@nushell nushell deleted a comment from github-actions bot May 12, 2025
@hustcer hustcer force-pushed the feature/msi-test branch from f77b2c5 to ddee6d3 Compare May 13, 2025 00:02
@github-actions
Copy link

Script Analysis

  • Key observations:
    • Addition of CI/CD workflows for MSI and Winget package testing
    • New test scripts (test-msi.nu, winget-install.nu) following Nushell best practices
    • Proper use of structured data handling (JSON, tables) in tests
    • Good module organization with dedicated test files
    • Compliance with Nu 0.90+ features (record syntax, pipeline operators)
    • Use of modern Nu commands like registry query

Security Review

  • Vulnerability findings:
    • ❗ Potential path injection risk in test-msi.nu where MSI path is constructed (line 9)
    • ⚠️ Sensitive information (installation logs) could be exposed in CI logs (commented debug lines)
    • ❗ Direct execution of downloaded binaries without verification in winget-install.nu (line 33)
    • ⚠️ Registry queries (--hkcu environment) could expose sensitive environment variables

Optimization Suggestions

  • Performance improvements:
    • Replace ls | where pattern with ls **/*.msi for more efficient file matching
    • Use parallel execution (par-each) for multiple assertion checks where possible
    • Cache downloaded MSI between test runs to avoid redundant downloads
    • Use open --raw instead of open -r for better performance with binary files
    • Consider lazy evaluation with do blocks for setup tasks

Overall Quality: 4 (Well-structured tests with minor security and performance improvements needed)

Recommended improvements:

  1. Add checksum verification for downloaded MSI files
  2. Sanitize path constructions using path join
  3. Add error handling for registry queries
  4. Implement cleanup steps in @after-all hooks

@nushell nushell deleted a comment from github-actions bot May 13, 2025
@nushell nushell deleted a comment from github-actions bot May 13, 2025
@hustcer hustcer marked this pull request as ready for review May 13, 2025 00:50
@hustcer hustcer merged commit 223aa12 into main May 13, 2025
17 checks passed
@hustcer hustcer deleted the feature/msi-test branch May 13, 2025 00:50
hustcer added a commit to nushell/nushell that referenced this pull request May 22, 2025
<!--
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.
-->
Publishing Nushell to winget has always been a challenge for us, and to
this day, many issues remain unresolved—and some seem almost impossible
to fix. The road to solving these problems may be winding and long, but
it's time for us to set out on this journey.

This PR try to fix the Windows arm64 release binaries and some `winget`
related issues:

- [x] Fixes #14815: build
Windows arm64 binaries by Windows arm64 runner
- [x] Upgrade WiX Toolset to latest 6.0 version: WiX 3 we used currently
doesn't support arm64 arch and [WiX v4 Security Fixes End Date is
2025/02/05](https://docs.firegiant.com/wix/)
- [x] Update the **nightly** workflow to make it work for all future
releases
- [x] Update the **release** workflow to make it work for all future
releases
- [x] Fixes #15698
- [x] Fixes #13719 so that
Nushell should be possible to be installed via winget with both user and
machine scope
- [x] Fixes #5927
- [x] Try to fix #14786
- [x] Fixes #9537

## Related but not planed issues:

- Related #13017
- Related #8053

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

- Nushell should be possible to be installed via winget with both user
and machine scope and The default should be user scope
  - User scope install by winget: `winget install Nushell.Nushell`
- User scope install by msiexec: `msiexec /i
nu-0.104.1-x86_64-pc-windows-msvc.msi /quiet /qn`
- Machine scope install by winget: `winget install Nushell.Nushell
--override 'ALLUSERS=1'`
- Machine scope install by msiexec: `msiexec /i
nu-0.104.1-x86_64-pc-windows-msvc.msi ALLUSERS=1`
- Note that `--scope` flag for `winget install` does not work use
`--override` instead
- Default user scope install dir:
`$'($nu.home-path)\AppData\Local\Programs\nu\'`
  - Default machine scope install dir: `C:\Program Files\nu\`
- When a standard user runs the installer and selects "Install for
everyone" (per-machine installation), Windows will automatically trigger
a UAC prompt, the user can enter admin credentials and the installation
will proceed with elevated privileges
- [hustcer/setup-nu](https://github.com/hustcer/setup-nu) should work
for `windows-11-arm` runners


# 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 toolkit.nu; toolkit test stdlib"` 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
> ```
-->

The latest MSI builds are available here:
https://github.com/nushell/nightly/releases/tag/v0.104.1
Actually all the nightly releases were built with latest changes
included: https://github.com/nushell/nightly/releases

`winget` and `msiexec` install tests goes here:
nushell/integrations#49

https://github.com/nushell/integrations/actions/runs/14974621061
https://github.com/nushell/integrations/actions/runs/14974621054

### Test winget install locally:
- git clone [email protected]:nushell/integrations.git
- User scope install: `winget install -m
manifests\n\Nushell\Nushell\0.104.1\`
- Run: `use tests\common.nu *; check-user-install`
- Machine scope install: `winget install -m
manifests\n\Nushell\Nushell\0.104.1\ --override 'ALLUSERS=1'`
- Run: `use tests\common.nu *; check-local-machine-install`

# 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.
-->

@fdncred I suggest releasing a patch version after merging this PR (only
the changes of this PR will be included) to ensure that the winget
release process works properly. This way, we can be more confident when
releasing version 0.105.0.

References:

-
https://learn.microsoft.com/en-us/windows/win32/msi/single-package-authoring
-
https://learn.microsoft.com/en-us/windows/package-manager/winget/source#add
-
https://github.com/microsoft/winget-pkgs/blob/master/doc/tools/SandboxTest.md
- https://docs.firegiant.com/quick-start/
-
https://docs.firegiant.com/wix3/tutorial/getting-started/putting-it-to-use/#_top
-
https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/msiexec#set-public-properties
hustcer added a commit to nushell/nushell that referenced this pull request May 22, 2025
<!--
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!
-->

<!--
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.
-->
Publishing Nushell to winget has always been a challenge for us, and to
this day, many issues remain unresolved—and some seem almost impossible
to fix. The road to solving these problems may be winding and long, but
it's time for us to set out on this journey.

This PR try to fix the Windows arm64 release binaries and some `winget`
related issues:

- [x] Fixes #14815: build
Windows arm64 binaries by Windows arm64 runner
- [x] Upgrade WiX Toolset to latest 6.0 version: WiX 3 we used currently
doesn't support arm64 arch and [WiX v4 Security Fixes End Date is
2025/02/05](https://docs.firegiant.com/wix/)
- [x] Update the **nightly** workflow to make it work for all future
releases
- [x] Update the **release** workflow to make it work for all future
releases
- [x] Fixes #15698
- [x] Fixes #13719 so that
Nushell should be possible to be installed via winget with both user and
machine scope
- [x] Fixes #5927
- [x] Try to fix #14786
- [x] Fixes #9537

- Related #13017
- Related #8053

<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

- Nushell should be possible to be installed via winget with both user
and machine scope and The default should be user scope
  - User scope install by winget: `winget install Nushell.Nushell`
- User scope install by msiexec: `msiexec /i
nu-0.104.1-x86_64-pc-windows-msvc.msi /quiet /qn`
- Machine scope install by winget: `winget install Nushell.Nushell
--override 'ALLUSERS=1'`
- Machine scope install by msiexec: `msiexec /i
nu-0.104.1-x86_64-pc-windows-msvc.msi ALLUSERS=1`
- Note that `--scope` flag for `winget install` does not work use
`--override` instead
- Default user scope install dir:
`$'($nu.home-path)\AppData\Local\Programs\nu\'`
  - Default machine scope install dir: `C:\Program Files\nu\`
- When a standard user runs the installer and selects "Install for
everyone" (per-machine installation), Windows will automatically trigger
a UAC prompt, the user can enter admin credentials and the installation
will proceed with elevated privileges
- [hustcer/setup-nu](https://github.com/hustcer/setup-nu) should work
for `windows-11-arm` runners

<!--
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 toolkit.nu; toolkit test stdlib"` 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
> ```
-->

The latest MSI builds are available here:
https://github.com/nushell/nightly/releases/tag/v0.104.1
Actually all the nightly releases were built with latest changes
included: https://github.com/nushell/nightly/releases

`winget` and `msiexec` install tests goes here:
nushell/integrations#49

https://github.com/nushell/integrations/actions/runs/14974621061
https://github.com/nushell/integrations/actions/runs/14974621054

- git clone [email protected]:nushell/integrations.git
- User scope install: `winget install -m
manifests\n\Nushell\Nushell\0.104.1\`
- Run: `use tests\common.nu *; check-user-install`
- Machine scope install: `winget install -m
manifests\n\Nushell\Nushell\0.104.1\ --override 'ALLUSERS=1'`
- Run: `use tests\common.nu *; check-local-machine-install`

<!-- 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.
-->

@fdncred I suggest releasing a patch version after merging this PR (only
the changes of this PR will be included) to ensure that the winget
release process works properly. This way, we can be more confident when
releasing version 0.105.0.

References:

-
https://learn.microsoft.com/en-us/windows/win32/msi/single-package-authoring
-
https://learn.microsoft.com/en-us/windows/package-manager/winget/source#add
-
https://github.com/microsoft/winget-pkgs/blob/master/doc/tools/SandboxTest.md
- https://docs.firegiant.com/quick-start/
-
https://docs.firegiant.com/wix3/tutorial/getting-started/putting-it-to-use/#_top
-
https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/msiexec#set-public-properties
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