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

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

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 directories
  • Make the changes blocked by secure settings

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.

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 - 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.
Relates to #1855
Relates to #3447
Relates to #2658
Relates to #3313 (not closing, because I want to fix this in a better way with custom destinations)
Possibly fixes #3655
Possibly fixes #3753

@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

@mrixner, the secure settings PR has been merged

@mrixner
Copy link
Contributor Author

mrixner commented Jun 14, 2025

OK. I can't work on this next week but will finish it when I have time.

@marticliment
Copy link
Owner

don't worry about timing 🙂.
Do it whenever you can and you want to.

@mrixner
Copy link
Contributor Author

mrixner commented Jun 17, 2025

For future reference-
I made the complex settings so related items could be centralized in the same logical setting, but I think it's probably too much overhead and not much gain to do something similar with the secure settings. I was going to just do the way it was before and use [PackageManager]SettingNames, but I realize that's not really as easily possible anymore with the new settings enums (which I personally like as a change) without hardcoding each package manager as a setting. Is that the implementation you would prefer I take, or some sort of resolve function that turns the package manager into the setting key, or do you have a different idea?

@marticliment
Copy link
Owner

I would go with a switch that enables the user to change this setting (see how command-line arguments are handled) , and then let this setting be a regular dictionary setting.

I think it would be too much of a hassle to have the user UAC each time the path is changed, and with only an initial toggle the point would be already achieved...

@marticliment
Copy link
Owner

Also, for the dictionary settings thing, you could go Setting[package manager]

@mrixner
Copy link
Contributor Author

mrixner commented Jul 1, 2025

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"

This works great, but requires an extra quotation mark to be added to the end of the command - not too big a deal until dealing with update / install commands and stuff. Using ^ to escape the spaces works in the terminal, but fails in UniGetUI saying cmd.exe^ : The term 'cmd.exe^' is not recognized as the name of a cmdlet, function, script file, or operable program. (even for the same command that worked in terminal). I don't really want to use the PS1 script, but it's probably a better solution than forcing random quotes on the commands for only Scoop and NPM (perhaps with an IsScript manager property) or just flat-out not letting you change those two, right?

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.

Chocolatey was not found after UnigetUI installation using chocolatey [BUG] Autodetect system chocolatey not always working
2 participants