Skip to content

Allow Executable Selection #3703

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

Merged
merged 44 commits into from
Jul 5, 2025
Merged

Conversation

mrixner
Copy link
Contributor

@mrixner mrixner commented May 30, 2025

  • I have read the contributing guidelines, and I agree with the Code of Conduct.
  • Have you checked that there aren't other open pull requests for the same changes?
  • Have you tested that the committed code can be executed without errors?
  • This PR is not composed of garbage changes used to farm GitHub activity to enter potential Crypto AirDrops.
    Any user suspected of farming GitHub activity with crypto purposes will get banned. Submitting broken code wastes the contributors' time, who have to spend their free time reviewing, fixing, and testing code that does not even compile breaks other features, or does not introduce any useful changes. I appreciate your understanding.

This PR allows the user a (limited, due to security concerns) ability to change the package manager executables used by the updater. It also makes it so the log page opens debug logs by default on debug builds.

TODO:

  • NPM, Scoop path retrieval
  • Chocolatey path concatenation
    • Re Chocolatey.cs:322: I'm not sure what this is doing, and thus I didn't port this. I also did not remove the setting from the telemetry, as the setting stays how it was before it was ported and it is useful information.
  • Correct PowerShell path retrieval
  • Search for chcoloatey in %ChocaletyInstall%
  • Search for NPM in NVM / FNM directories (not doing this, as it appears to install them in normal NodeJS locations)
  • Make the changes blocked by secure settings
  • Explicitly search for some default installation directories, like C:\ProgramData\Chocolatey

I would suggest mentioning that the system chocolatey stuff has changed in the release notes. I had it automatically port old "UseSystemChocolatey" configs to setting the default path.

I'm also slightly concerned about the changes to how NPM and Scoop are invoked not working on all systems - I'll monitor issues with the next beta to ensure it works with all configurations and do my best to fix it if it doesn't work.

Sorry for all the formatting-only changes, my editor started autoformatting and I don't really know how to turn it off...


I don't think it closes these issues, specifically the FNM ones, because those are really about being able to choose any path, since I don't know if the FNM install directory is located by UniGetUI The UseSystemChocolatey toggle will remove the "unigetui" chocolatey install from the list of available chocolateys

  • However, there is now a simple fix for people with similar issues, which is to add the standard install directories for their tools to the search path in LoadAvailablePaths.

fix #1855
fix #3447
fix #2658
fix #3313
fix #3400

@mrixner mrixner force-pushed the select-executables branch from f3096e3 to 9d29cf6 Compare May 30, 2025 16:58
@mrixner
Copy link
Contributor Author

mrixner commented May 30, 2025

@marticliment - I have a question about the way certain package managers run their commands. NPM and Scoop both seem to use PowerShell to run their commands instead of running them directly, whereas no other package manager does it that way. What was the reasoning behind this? Was it that whole thing with PowerShell where stuff adds "PowerShell Aliases" instead of actually properly installing it?

Basically what I'm asking is is it necessary to keep that in, or does the where search suffice? Because otherwise the current plan ends up breaking down for these two as it's looking for executables, which would be PowerShell as opposed to NPM or Scoop.


Also, the way this PR is implemented would make it fairly simple for system chocolatey and bundled chocolatey to just be two parts of the same selection, removing the need for a "use system chocolatey" checkbox. Would you want that, or do you think it would be too confusing and the checkbox should stay?

We can have it use bundled chocolatey if there's nothing else, since that's the only path, or use system chocolatey if one was found, which I think was something requested and does make sense (to have bundled chocolatey be only for users without chocolatey installed).

@mrixner
Copy link
Contributor Author

mrixner commented May 30, 2025

Also, tests are failing due to missing language key es-MX, I'm not sure what that means..

@mrixner
Copy link
Contributor Author

mrixner commented May 30, 2025

Is there a way to show a restart required banner for the package manager settings? It doesn't seem like the function is still there...

@marticliment
Copy link
Owner

@marticliment - I have a question about the way certain package managers run their commands. NPM and Scoop both seem to use PowerShell to run their commands instead of running them directly, whereas no other package manager does it that way. What was the reasoning behind this? Was it that whole thing with PowerShell where stuff adds "PowerShell Aliases" instead of actually properly installing it?
Basically what I'm asking is is it necessary to keep that in, or does the where search suffice? Because otherwise the current plan ends up breaking down for these two as it's looking for executables, which would be PowerShell as opposed to NPM or Scoop.

Yes, it is. This happens because scoop and npm are not executable files, but rather scripts, so on certain conditions they wouldn't work as expected, unless run from a shell such as cmd or powershell

@marticliment
Copy link
Owner

Also, the way this PR is implemented would make it fairly simple for system chocolatey and bundled chocolatey to just be two parts of the same selection, removing the need for a "use system chocolatey" checkbox. Would you want that, or do you think it would be too confusing and the checkbox should stay?

We can have it use bundled chocolatey if there's nothing else, since that's the only path, or use system chocolatey if one was found, which I think was something requested and does make sense (to have bundled chocolatey be only for users without chocolatey installed).

I guess this makes sense.

@marticliment
Copy link
Owner

Also, tests are failing due to missing language key es-MX, I'm not sure what that means..

Don't worry about this, I'll be fixing this soon on main

@marticliment
Copy link
Owner

Is there a way to show a restart required banner for the package manager settings? It doesn't seem like the function is still there...

Yes, you should be able to invoke the RestartRequired?.Invoke(this, EventArgs.Empty) event, if I recall properly

@mrixner
Copy link
Contributor Author

mrixner commented May 31, 2025

I'm unable to coerce PowerShell into properly executing the custom script paths; I updated the executed command to effectively be C:\WINDOWS\system32\windowspowershell\v1.0\powershell.exe -NoProfile -ExecutionPolicy Bypass -Command "& \"C:\Program Files\nodejs\npm\" outdated --json --global" but doing so creates a cmd window for every command run. & triggers the custom script correctly, but flashes a command prompt window, despite it working properly from the terminal. Is there a way you've bypassed this elsewhere in the application?

I could have it only use the npm PS1 scripts, but I don't know if every npm installation provides one...

@marticliment
Copy link
Owner

marticliment commented Jun 1, 2025

It looks like the .ps1 script is bundled with nodejs. So I'd give it a try

Perhaps, anothet possible solution could be running through an inner command prompt C:\WINDOWS\system32\windowspowershell\v1.0\powershell.exe -NoProfile -ExecutionPolicy Bypass -Command "cmd.exe /C \"C:\Program Files\nodejs\npm\" outdated --json --global"

@mrixner
Copy link
Contributor Author

mrixner commented Jun 2, 2025

Does scoop have a ps1 script? I don't have it installed.

I feel like cmd is probably more robust if the users want to change their npm location... I'll try it when I can and see if they work.

@marticliment
Copy link
Owner

I feel like cmd is probably more robust if the users want to change their npm location... I'll try it when I can and see if they work.

Give it a try, but I have experienced issues with CMD and encoding, so please be careful there. At CoreData, there is a CodePage (or Encoding or sth property), which loads the user's codepage, to build a compatible encoding to pass to Process(). Then, special characters will work as expected.

@marticliment
Copy link
Owner

@mrixner, I have created #3722, to finally implement the everlasting debate about insecure settings. Once that PR is merged, please block this feature behind a new secure setting. Thanks

@mrixner
Copy link
Contributor Author

mrixner commented Jun 7, 2025

Should I change it to allow the selection of any executable, or leave it as a dropdown?

@marticliment
Copy link
Owner

I would leave it as a drop down

@marticliment
Copy link
Owner

I am sorry @mrixner, #3722 was closed accidentally. The new PR is #3723

@marticliment
Copy link
Owner

Oh, do you mean the Scoop check in LoadAvailablePaths where it loads scoop PS1s from all the scoops in the path? That's not a fallback, that's what actually allows the multiple selection, if there are.

To be honest, I don't think it is necessary for the user to have the choice between both scoop and scoop.ps1. Since both npm and scoop use the file to invoke the file.ps1, I will do some testing on how it would work if we'd only use .ps1 files. If it works reliably, I will push a commit here. I'd prefer the -File approach because it is much more secure than using the -Command.

@marticliment
Copy link
Owner

marticliment commented Jul 4, 2025

That whole spaces thing is why I thing that this PR specifically really needs to be in a well-tested beta

Yes, I plan to publish 3.2.1-beta3 after merging this PR

@mrixner
Copy link
Contributor Author

mrixner commented Jul 4, 2025

Oh, do you mean the Scoop check in LoadAvailablePaths where it loads scoop PS1s from all the scoops in the path? That's not a fallback, that's what actually allows the multiple selection, if there are.

To be honest, I don't think it is necessary for the user to have the choice between both scoop and scoop.ps1.

It doesn't provide an option for scoop, it is just finding scoop executables on the path and adding the PS1 version to the options, solely in case where doesn't return the PS1 properly. But you're right, I forgot where works with PS1s so it shouldn't be necessary.

@marticliment
Copy link
Owner

I have also found that, when using the -File argument, unless StandardInput is closed from C# the process hangs. I will continue investigating this

@marticliment marticliment merged commit 7f43cf5 into marticliment:main Jul 5, 2025
2 checks passed
@mrixner
Copy link
Contributor Author

mrixner commented Jul 5, 2025

Three things:

One, In GetExecutableFile, you changed it to only allow items in the path, as "only path is searched" - however this is not true, as vcpkg (the more likely one to get messed up by this) is searched for in the VCPKG_ROOT directory and Python is searched for in standard installation directories and I was thinking about doing that for some other managers as well, since that's what most of the open issues are about, meaning as new ones are opened for management tools / new standard directories they can just be added to the search list, but that check would break this.

I assume you did this for security so someone can't sneak a random executable in?

Two, I was migrating system Chocolatey explicitly for anyone who had system chocolatey installed but was preferring to / didn't realize they were using UniGetUI's; if this change is made, system Chocolatey can still see the packages installed by UniGetUI Chocolatey, right?

Three, I forgot winget had a bundled option like Chocolatey, or I would've done the same thing for it; why not just allow the user to choose bundled by changing the path, removing the need for the checkbox?

@marticliment
Copy link
Owner

One, In GetExecutableFile, you changed it to only allow items in the path, as "only path is searched" - however this is not true, as vcpkg (the more likely one to get messed up by this) is searched for in the VCPKG_ROOT directory and Python is searched for in standard installation directories and I was thinking about doing that for some other managers as well, since that's what most of the open issues are about, meaning as new ones are opened for management tools / new standard directories they can just be added to the search list, but that check would break this.
I assume you did this for security so someone can't sneak a random executable in?

I was thinking that it would be easy to have a network executable file set as the manager, hence the limitation, but I guess I can manually filter those out and not have a path-only limitation.

Two, I was migrating system Chocolatey explicitly for anyone who had system chocolatey installed but was preferring to / didn't realize they were using UniGetUI's; if this change is made, system Chocolatey can still see the packages installed by UniGetUI Chocolatey, right?

I saw the migration, but I reckon it is easier to maintain both switches. Enabling system chocolatey will just remove the unigetui chocolatey from path, leaving the next system path as the first executable, essentially being the one selected

Three, I forgot winget had a bundled option like Chocolatey, or I would've done the same thing for it; why not just allow the user to choose bundled by changing the path, removing the need for the checkbox?

WinGet requires some more care, since the switch does not only use the local executable (which, when enabled, will be left the first available file). It also forces disabling the COM API and using console-based parsing.

@marticliment
Copy link
Owner

In reality, the check was being made against the candidate executables (so it would work with vcpkg or python, as it does with local winget, that is not in path). However, the comments and labels were misleading, and I have just corrected that

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces user-configurable executable selection for package managers and makes the log page open in debug level by default on debug builds.

  • Adds an Executable file picker to the Package Manager settings, gated by a secure setting.
  • Implements FindCandidateExecutableFiles and GetExecutableFile in the core PackageManager class and updates all manager implementations to use these.
  • Defaults the log page to debug verbosity in debug builds.

Reviewed Changes

Copilot reviewed 39 out of 39 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/UniGetUI/Pages/SettingsPages/ManagersPages/PackageManager.xaml(.cs) Adds UI and code for selecting manager executable paths.
src/UniGetUI/Pages/SettingsPages/GeneralPages/Administrator.xaml(.cs) Introduces and wires up AllowCustomManagerPaths secure setting.
src/UniGetUI/Pages/LogPages/UniGetUILogPage.cs Sets debug log level by default in debug builds.
src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/PackageManager.cs Defines candidate discovery and selection of executables.
src/UniGetUI.Core.Tools/Tools.cs Adds WhichMultiple helper for finding all occurrences in PATH.
Multiple PackageEngine.Managers.* classes Switches to new path/arg logic via Status.ExecutableCallArgs.
Comments suppressed due to low confidence (2)

src/UniGetUI.Core.Tools/Tools.cs:112

  • [nitpick] The new WhichMultiple method lacks an XML documentation comment. Adding a <summary> and <returns> docblock will improve discoverability and maintain consistency with other helpers.
        public static List<string> WhichMultiple(string command, bool updateEnv = true)

src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/PackageManager.cs:125

  • The new methods FindCandidateExecutableFiles and GetExecutableFile contain important path-selection logic. Consider adding unit tests to validate behavior with multiple candidates, missing files, and secure-settings toggles.
        public abstract IReadOnlyList<string> FindCandidateExecutableFiles();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment