-
-
Notifications
You must be signed in to change notification settings - Fork 560
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
Conversation
NPM, Scoop, Chocolatey, and PowerShell have not been implemented as they are awaiting clarification.
f3096e3
to
9d29cf6
Compare
@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 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). |
Also, tests are failing due to missing language key |
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, 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 |
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. |
Don't worry about this, I'll be fixing this soon on main |
Yes, you should be able to invoke the |
…nd not the path-infused bundled chocolatey
I'm unable to coerce PowerShell into properly executing the custom script paths; I updated the executed command to effectively be I could have it only use the npm PS1 scripts, but I don't know if every npm installation provides one... |
It looks like the Perhaps, anothet possible solution could be running through an inner command prompt |
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. |
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. |
Should I change it to allow the selection of any executable, or leave it as a dropdown? |
I would leave it as a drop down |
To be honest, I don't think it is necessary for the user to have the choice between both |
Yes, I plan to publish 3.2.1-beta3 after merging this PR |
…andomly changed when selecting the first appearance
It doesn't provide an option for |
…ExecutableCallArguments (so they can be loaded dynamically)
I have also found that, when using the -File argument, unless StandardInput is closed from C# the process hangs. I will continue investigating this |
…be added by not adding the local executable as the first one, so the default file will be pulled from path
Three things: One, In 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? |
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.
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
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. |
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 |
There was a problem hiding this 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
andGetExecutableFile
in the corePackageManager
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
andGetExecutableFile
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();
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:
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.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 UniGetUIThe UseSystemChocolatey toggle will remove the "unigetui" chocolatey install from the list of available chocolateysLoadAvailablePaths
.fix #1855
fix #3447
fix #2658
fix #3313
fix #3400