Skip to content

Support Windows via pwsh - Powershell needs to be installed manually#77

Open
azriel91 wants to merge 6 commits into
Aloso:mainfrom
azriel91:feature/3/windows-powershell
Open

Support Windows via pwsh - Powershell needs to be installed manually#77
azriel91 wants to merge 6 commits into
Aloso:mainfrom
azriel91:feature/3/windows-powershell

Conversation

@azriel91

@azriel91 azriel91 commented May 17, 2025

Copy link
Copy Markdown

If we use the Powershell binary, it defaults to Powershell 1.0, which doesn't have ANSI support; I think it only got introduced in Powershell 5.0.

After installing powershell manually, pwsh is available on the PATH.

I know some windows users will have git-bash installed, but I don't, so as a first cut I just added support for pwsh.

I added tests for windows; and I changed the current working directory restoration to not use pwd, so it would be good to test on a non-windows system with a command that changes the working dir.

Screenshot:

image

to-html 'cargo build --color=always'
<!-- added this for the screenshot -->
<style>
.terminal {
   background: black;
   color: white;
   border-radius: 4px;
   padding: 10px;
}
</style>

<!-- this was the output -->
<pre class="terminal">
<span class='shell'>&gt; </span><span class='cmd'>cargo</span> <span class='arg'>build</span> <span class='flag'>--color</span><span class='arg'>=always</span>
<b><span style='color:var(--green,#0a0)'>   Compiling</span></b> simple v0.1.0 (C:\Users\azrielh\AppData\Local\Temp\simple)
<b><span style='color:var(--bright-red,#f55)'>error[E0382]</span><span style='color:var(--bright-white,#fff)'>: use of moved value: `hello`</span></b>
  <b><span style='color:var(--bright-cyan,#5ff)'>--&gt; </span></b>src\main.rs:10:11
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>
<b><span style='color:var(--bright-cyan,#5ff)'>6</span></b>  <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>     let hello = String::from(&quot;hello&quot;);
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>         <b><span style='color:var(--bright-cyan,#5ff)'>-----</span></b> <b><span style='color:var(--bright-cyan,#5ff)'>move occurs because `hello` has type `String`, which does not implement the `Copy` trait</span></b>
<b><span style='color:var(--bright-cyan,#5ff)'>...</span></b>
<b><span style='color:var(--bright-cyan,#5ff)'>9</span></b>  <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>     print(hello);
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>           <b><span style='color:var(--bright-cyan,#5ff)'>-----</span></b> <b><span style='color:var(--bright-cyan,#5ff)'>value moved here</span></b>
<b><span style='color:var(--bright-cyan,#5ff)'>10</span></b> <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>     print(hello);
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>           <b><span style='color:var(--bright-red,#f55)'>^^^^^</span></b> <b><span style='color:var(--bright-red,#f55)'>value used here after move</span></b>
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>
<b><span style='color:var(--bright-green,#5f5)'>note</span></b>: consider changing this parameter type in function `print` to borrow instead if owning the value isn&#39;t necessary
  <b><span style='color:var(--bright-cyan,#5ff)'>--&gt; </span></b>src\main.rs:1:13
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>
<b><span style='color:var(--bright-cyan,#5ff)'>1</span></b>  <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b> fn print(s: String) {
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>    <b><span style='color:var(--bright-cyan,#5ff)'>-----</span></b>    <b><span style='color:var(--bright-green,#5f5)'>^^^^^^</span></b> <b><span style='color:var(--bright-green,#5f5)'>this parameter takes ownership of the value</span></b>
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>    <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>    <b><span style='color:var(--bright-cyan,#5ff)'>in this function</span></b>
<b><span style='color:var(--bright-cyan,#5ff)'>help</span></b>: consider cloning the value if the performance cost is acceptable
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>
<b><span style='color:var(--bright-cyan,#5ff)'>9</span></b>  <b><span style='color:var(--bright-cyan,#5ff)'>| </span></b>    print(hello<span style='color:var(--bright-green,#5f5)'>.clone()</span>);
   <b><span style='color:var(--bright-cyan,#5ff)'>|</span></b>                <span style='color:var(--bright-green,#5f5)'>++++++++</span>

<b><span style='color:var(--bright-white,#fff)'>For more information about this error, try `rustc --explain E0382`.</span></b>
<b><span style='color:var(--red,#a00)'>error</span>:</b> could not compile `simple` (bin &quot;simple&quot;) due to 1 previous error
<span class='shell'>&gt; </span><span class='caret'> </span>
</pre>

@azriel91 azriel91 mentioned this pull request May 17, 2025
9 tasks
@CosmicHorrorDev
CosmicHorrorDev self-requested a review May 17, 2025 08:25
@CosmicHorrorDev

Copy link
Copy Markdown
Collaborator

Thanks for the PR. I'll try to give it a review tomorrow 👀

Comment thread crates/fake-tty/src/lib.rs Outdated

#[cfg(target_os = "windows")]
{
Ok(out)

@Aloso Aloso May 17, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This can contain carriage returns (\r) according to the tests, so it should replace them like in line 121.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ah, wasn't sure if we wanted \rs in Windows land.
Shall remove them.

Comment thread src/cmd.rs Outdated
if !cmp_paths(std::env::current_dir()?, cwd) {
std::env::set_current_dir(cwd)?;
if !cmp_paths(std::env::current_dir()?, &cwd_before) {
std::env::set_current_dir(cwd_before)?;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

That's not what the original code did. When you do

to-html "cd .." "ls"

The 1st command changes directory, and the 2nd command should print the entries in that directory. The problem was that running cd in fake_tty::command does not affect the working directory of the current process, so I wrote this "hack".

@CosmicHorrorDev CosmicHorrorDev May 18, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Expressed this behavior in a test with this commit (f65923c). It indeed succeeds on main, but fails with this PR

(force pushed to fix mac and linux using slightly different formatting for ls output)

@CosmicHorrorDev CosmicHorrorDev May 18, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Whelp that's odd. That change appears to fail consistently on only the ubuntu, rustc beta job with output that looks identical to not actually changing the current working directory. I'm not able to reproduce it locally when running the 1.88.0-beta.3 toolchain on Arch :/

https://github.com/CosmicHorrorDev/to-html/actions/runs/15090969623/job/42419546987

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ah, looks like I've never changed directories when using to-html!

I've fixed the directory changing code -- getting it to work on Windows is a little hacky (std::env::set_current_dir doesn't like pwd outputs, which are different depending on which bash you're running).

@CosmicHorrorDev I've taken that commit and added it to this branch (and used ls -1 to avoid the \t issue).

@Aloso

Aloso commented May 17, 2025

Copy link
Copy Markdown
Owner

Also, I believe this will means that to-html can't be used with git-bash on Windows.

What I'd expect is that to-html "some command" uses bash (on all platforms), and to-html "some command" --shell=pwsh uses PowerShell (note that PowerShell can also be installed on Linux, it is not Windows specific).

@azriel91

Copy link
Copy Markdown
Author

By the way, the default bash finds the WSL bash, and always invokes that; I tried with git-bash, and had to add it to PATH for it to resolve:

The following are WSL bash, then git-bash, run through a git-bash cmd prompt:

image

@Aloso

Aloso commented May 19, 2025

Copy link
Copy Markdown
Owner

Thank you for working on this! I'll have to test it more thoroughly.

@azriel91
azriel91 force-pushed the feature/3/windows-powershell branch from 29e6eb5 to a5ced54 Compare May 20, 2025 04:49
@CosmicHorrorDev

Copy link
Copy Markdown
Collaborator

I'll leave the review up to @Aloso since it seems like he's up for it 🎉

One thing that I would like is if we could add windows in our test.yml CI workflow and (potentially in some other PR) to our release.yml workflow that handles building releases

@CosmicHorrorDev
CosmicHorrorDev requested review from Aloso and removed request for CosmicHorrorDev May 20, 2025 05:22
@azriel91

Copy link
Copy Markdown
Author

Heya @Aloso, would you mind if I added an Error enum with variants for every ? used?

The run_changes_directories test seems to have a race condition on Windows -- when I run cargo test --workspace it passes about 80% of the time, but the failure only shows "" != "<expected stdout>", and the error origin location is lost because it propagates through the ?, and it's really hard to trace the cause just using my brain.

(I haven't checked if there's a way to get the origin location by looking for the error's backtrace)

@Aloso

Aloso commented May 23, 2025

Copy link
Copy Markdown
Owner

@azriel91 you can do that. But what I think would help more is to add anyhow, then you can use .context(...)? to get more information.

@Aloso

Aloso commented May 23, 2025

Copy link
Copy Markdown
Owner
  • Documentation needs updating. I'll take care of that when this PR is ready.

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