Skip to content
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

Fix WinUtil admin elevation for stable and pre-release builds #2908

Closed

Conversation

Cryostrixx
Copy link
Contributor

@Cryostrixx Cryostrixx commented Oct 10, 2024

Contributor Note

This PR has been marked as closed and deleted in favor of ruxunderscore's PR ID No. 2913. This PR's branch has been removed.
I've suggested the fixes I made for Rux's PR in start-wt-fix-proposed and start-wt-fix-experimental so this PR is redundant.

@Cryostrixx Cryostrixx marked this pull request as ready for review October 10, 2024 07:36
Copy link
Contributor

@og-mrk og-mrk left a comment

Choose a reason for hiding this comment

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

Although these changes are great, and I do see the effort you've put into this PR.. but downloading WinUtil into Temp folder is something we prefer not doing, and instead downloading WinUtil into RAM (piping result of IRM into IEX). Or in other words, if we can avoid creating Artifact Files, then we should try to avoid it.

@Cryostrixx
Copy link
Contributor Author

Cryostrixx commented Oct 10, 2024

Hello, @og-mrk!

I appreciate your feedback on my PR, but I want to clarify that I’ve worked hard to ensure the script functions correctly. The current version successfully addresses the issues numerous users have faced and meets the intended goals. Therefore, I will not be pursuing in-memory execution, as it has caused problems with running the script in the past.

I hope we can focus on solutions that enhance functionality rather than reverting to behavior that resulted in a non-functioning script.

I'd also like to mention that when using Start-Process, the -WorkingDirectory parameter is passed by default, preventing issues like this from happening and ensuring the script works as intended:
image

Thank you for your understanding, and I look forward to your feedback!


Additional Notes:

  • I want to clarify that the primary goal of this PR is to ensure the script works effectively without changing functionality.
    Using in-memory execution has caused nothing but problems in the past, as illustrated in the screenshot I shared above.
  • I've also improved how the raw UI's window title is set and have requested your review for those specific code changes.

@ruxunderscore
Copy link
Contributor

Using in-memory execution has caused nothing but problems in the past, as illustrated in the screenshot I shared above.

What issues have there been with executing from in-memory? At the end of the argument, keep in mind that we chose to keep this running in this way to keep from having to store artifacts, as @og-mrk has mentioned, and it's up to Chris whether or not this is changed.

@Cryostrixx
Copy link
Contributor Author

Cryostrixx commented Oct 10, 2024

Using in-memory execution has caused nothing but problems in the past, as illustrated in the screenshot I shared above.

What issues have there been with executing from in-memory? At the end of the argument, keep in mind that we chose to keep this running in this way to keep from having to store artifacts, as @og-mrk has mentioned, and it's up to Chris whether or not this is changed.

I understand your point about wanting to not use temporary files but is it really preferable to have a non-functional script?
See attached screenshot:
image

With my fix in place, it works as it was intended to, which you can see illustrated in the screenshot that I have attached below:
image

@ruxunderscore
Copy link
Contributor

This is a launch error with Windows Terminal, not the fact that it's run in Ram. I am looking into a fix, now.

@Cryostrixx
Copy link
Contributor Author

Cryostrixx commented Oct 10, 2024

@ruxunderscore So what should I do with the PR? I don't want to close it yet, as I spent one week just ensuring the fix works.

@Cryostrixx Cryostrixx marked this pull request as draft October 10, 2024 22:53
@ruxunderscore
Copy link
Contributor

@ruxunderscore So what should I do with the PR? I don't want to close it yet, as I spent one week just ensuring the fix works.

Keeping it as a draft should be fine. Once I figure out how generate my own winutil.ps1 within my own fork, I can do more testing.

@Cryostrixx
Copy link
Contributor Author

Cryostrixx commented Oct 10, 2024

@ruxunderscore So what should I do with the PR? I don't want to close it yet, as I spent one week just ensuring the fix works.

Keeping it as a draft should be fine. Once I figure out how generate my own winutil.ps1 within my own fork, I can do more testing.

You should be able to run these commands to do it, it's the same commands I use to pull in the changes from my fork:

sl C:\path\to\your\winutil\fork
git checkout your-feature-branch-name
.\Compile.ps1 -SkipPreprocessing
.\winutil.ps1

If you want to pull the changes from my feature branch into your local repo you would run the following commands:

sl C:\path\to\your\winutil\fork
git remote add Cryostrixx_winutil https://github.com/Cryostrixx/winutil
git fetch Cryostrixx_winutil
git checkout fix-winutil-admin-elevation
.\Compile.ps1 -SkipPreprocessing
.\winutil.ps1

If you want to remove the changes and sync your fork with the main branch you would run the following commands:

git remote remove Cryostrixx_winutil
git checkout main
git branch -D fix-winutil-admin-elevation
git push origin --delete fix-winutil-admin-elevation

Here's a screenshot showing these commands in effect:
image


Just make sure to replace C:\path\to\your\winutil\fork with the full path to your fork, replace your-feature-branch-name with the name of the feature branch that contains your changes, and ensure that you pushed your changes to your fork with git push origin your-feature-branch-name (you can check this with git reflog).
Note: I can't guarantee these commands will work on your system as I don't know what your configuration is, if you have a different configuration feel free to adjust these commands according to your setup.

@ruxunderscore
Copy link
Contributor

ruxunderscore commented Oct 11, 2024

You should be able to run these commands to do it, it's the same commands I use to pull in the changes from my fork:

I ended doing a partial rollback of changes in an older PR, as I recalled that elevation was working in past for me. Only did changes for start.ps1, for now, but may take a look at windev.ps1 later to try to do a partial rollback there as well. Checkout my PR #2913, and let me know if self-elevation works for you, now.

@Cryostrixx
Copy link
Contributor Author

Cryostrixx commented Oct 11, 2024

I'm just curious: What even broke? I've been using WinUtil for a while and there was a time when this worked without any issue.
I'll test it now and let you know how it goes.

@ruxunderscore
Copy link
Contributor

I'm just curious: What even broke? I've been using WinUtil for a while and there was a time when this worked without any issue. I'll test it now and let you know how it goes.

After comparing the changes, the only thing I can think of is the handling of arguments.

@Cryostrixx
Copy link
Contributor Author

Cryostrixx commented Oct 11, 2024

@ruxunderscore It ran perfectly when testing it locally. No errors there, but there are still issues trying to run it using irm | iex:
image


Update: I forgot to checkout the branch, it runs with no issues but when passing -Config ~\Documents\Configs\winutil.json my configuration is not imported.

@ruxunderscore
Copy link
Contributor

Interesting. I was hosting my ps1 script on another system I have running a webserver and didn't see that issue after my changes. Is this after a compile? And did you remove the old winutil.ps1 if it was still there?

@Cryostrixx
Copy link
Contributor Author

Cryostrixx commented Oct 11, 2024

@ruxunderscore See the latest update. I forgot to check out the branch, I'll test it again and let you know how it goes.
Update: I just tested it, it launches fine but my configuration doesn't seem to be getting imported when I pass -Config ~\Documents\Configs\winutil.json.

@ruxunderscore
Copy link
Contributor

@ruxunderscore See the latest update. I forgot to check out the branch, I'll test it again and let you know how it goes.
Update: I just tested it, it launches fine but my configuration doesn't seem to be getting imported when I pass -Config ~\Documents\Configs\winutil.json.

I believe that was the original intent on the handling of Arguments the way they were, but at this point it's either it runs or doesn't. We can work on a solution for the arguments later.

@Cryostrixx Cryostrixx closed this Oct 12, 2024
@Cryostrixx Cryostrixx deleted the fix-winutil-admin-elevation branch October 12, 2024 06:32
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.

3 participants