marticliment
Allow Executable Selection
- [x] I have read the contributing guidelines, and I agree with the Code of Conduct.
- [x] Have you checked that there aren't other open pull requests for the same changes?
- [x] Have you tested that the committed code can be executed without errors?
- [x] 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
- [x] 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.
- Re
- [ ] Correct PowerShell path retrieval
- [ ] Search for chcoloatey in %ChocaletyInstall%
- [ ] Search for NPM in NVM directories
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
@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).
Also, tests are failing due to missing language key es-MX, I'm not sure what that means..
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 - 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
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.
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
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
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...
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"
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.
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.
@mrixner, I have created https://github.com/marticliment/UniGetUI/pull/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
Should I change it to allow the selection of any executable, or leave it as a dropdown?
I am sorry @mrixner, #3722 was closed accidentally. The new PR is #3723
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?
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...
Also, for the dictionary settings thing, you could go Setting[package manager]
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?
I'd also go .ps1 script instead of having a double powershell-cmd command execution
Everything is, I believe, implemented and tested. Let me know if you find any problems!
@mrixner, for running scoop and npm, which are .ps1 files, wouldn't it be easier to do the following?
Idk if you have tested this or not, but if it is an option it would make it harder for command injection to happen on powershell-called executables. What do you think?
If you agree I can change that to using the file
(Edit, I am thinking also on both scoop and npm to invoke only .ps1 files, and do not have a fallback on scoop and ps1 files, since the .ps1 files are the way in which powershell calls scoop and npm, so they are not going to be removed or renamed)
I did attempt that, and while it works on the command line it did not work in UniGetUI. I believe that one was the error where it was complaining about having spaces in the path, even though I escaped / quoted them and it worked in the command line, copy pasting the command that was run; I'm not sure, but I know it didn't work in UniGetUI for some reason. I tried pretty much everything I could find and the current solution is the only one I could get to work 😅. You can try and see if I was doing something wrong if you would like; you're right, it's probably a better way to do it, if it works.
That whole spaces thing is why I thing that this PR specifically really needs to be in a well-tested beta - if I hadn't had a space in my path and it had work, something would've broken for someone. I don't know how many of those are there that I didn't know about when testing.
(Edit, I am thinking also on both scoop and npm to invoke only .ps1 files, and do not have a fallback on
scoopandps1files, since the .ps1 files are the way in which powershell calls scoop and npm, so they are not going to be removed or renamed)
What do you mean by having a "fallback on scoop and ps1 files"?
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.
In fact, I should probably do that for npm as well if it's not there already.