-
Notifications
You must be signed in to change notification settings - Fork 168
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
Windows: Report installation messages to console #847
Conversation
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.
I like this a lot. However, I think it there should be an option to suppress the output to still allow for old behavior, especially since stream redirecting doesn't work.
Alternatively, we can make this an opt-in option, but I think outputting into the console should be the default.
Do you know how this would look like inside a GitHub Action?
Good point. You mean a CLI flag (e.g.
Haven't checked yet, but will do. |
A CLI flag |
The output doesn't show up in GHA logs. I guess their capture mechanism doesn't involve low level Windows APIs and "just" the stdout streams 😂 Or am I missing something in how I set this up? |
Ok, it looks like stream redirection does work if invoked directly like:
However that call is not blocking, so the user will have to check the logs manually via polling or something. Added this gotcha to the docs. |
You can see that the direct call does work on GHA output now, but we have to block with some sleep-like statements. |
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.
I discovered another gotcha: calling the uninstaller opens a new Window for some reason.
docs/source/cli-options.md
Outdated
like this: | ||
|
||
```batch | ||
> cmd.exe /c start /wait my_installer.exe /S |
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.
cmd.exe /c
is not necessary. You could also consider adding PowerShell since this is the default profile in the Windows Terminal
Start-Process -FilePath my_installer.exe -ArgumentList "/S" -NoNewWindow -Wait
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.
Nice, you can even use -RedirectStandardOutput out.txt
to correctly redirect things!
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.
Added this to the documentation.
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.
It looks like you replaced the documentation. I don't see the harm showing both cmd.exe
and PowerShell.
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.
Added tabs with both now.
Yep, can reproduce. I think this is due to the fact that the uninstaller copies itself to a temporary location so it can delete itself during the "Removing files and folders..." step. That would cause the new process to lose the parent console and hence why we get a new allocated console for that. I guess that's better than nothing so I'm inclined to leave it like that. I'm not concerned about CI usage because in those cases users don't really uninstall, just spin a new one up, right? |
I use the uninstallation procedure in a CI, but I don't see how this would break it. And even if it does, However, it might be worth pointing out the new window in the documentation. |
Added a couple sentences. |
> cmd.exe /c start /wait my_installer.exe /S | ||
`````{tab-set} | ||
````{tab-item} CMD | ||
```pwsh |
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.
```pwsh | |
```batch |
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.
I used pwsh on purpose here because the batch syntax highlighting didn't shown any colours at all.
|
||
`````{tab-set} | ||
````{tab-item} CMD | ||
```pwsh |
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.
```pwsh | |
```batch |
> cmd.exe /c start /wait my_installer.exe /S | ||
`````{tab-set} | ||
````{tab-item} CMD | ||
```pwsh |
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.
```pwsh | |
```batch |
> my_installer.exe /S > logs.txt | ||
`````{tab-set} | ||
````{tab-item} CMD | ||
```pwsh |
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.
```pwsh | |
```batch |
Description
/S
flag doesn't include any feedback #812Taking inspiration from the examples collected in #764 (comment), we can print directly to the attached console (which might not be the stdout stream!) and report some information. This mostly works but we can't rely on stream redirection, piping or capture (that's why we still need the log files for testing) unless we execute the installer directly without
START /WAIT
. For most end users, this won't be an issue, I hope. The output can be silenced with/Q
(quiet).This only applies to
/S
mode (S for silent, but let's call it headlesS now).This is how it looks more or less:
Checklist - did you ...
news
directory (using the template) for the next release's release notes?