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

feat(windows) powershell and batch support #45

Conversation

tensor-programming
Copy link
Contributor

@tensor-programming tensor-programming commented Mar 6, 2020

Which issue does this fix?

Closes #10

Describe the solution

Added batch and powershell scripting for windows.

Powershell worked out of the box with some minor idiosyncrasies (more on that in a moment) and batch just required a new match statement for the runtime. CFG was used for batch and for a few of the tests. Each instance of shell/bash in the tests and maskfile.md was given a complimentary powershell script as well. On the Linux/WSL side, had to make some changes to the parser because it didn't ignore the powershell and batch blocks. Used CFG to change the way the parser matches on codeblock tags.

Regarding Powershell: normally when using powershell you are not allowed to pass double hyphen flags -- to a script. This somewhat dubious choice doesn't seem to affect mask since the arguments are passed into the environment. If you pass a flag x as such --x you are able to gain access to the value of the flag via $env:x. This works out nicely because there doesn't need to be two separate CLI interfaces for windows and Linux.

The Paths in windows are proceeded by \\?\, for instance: \\?\C:\\path\\to\\maskfile.md. If this prefix is removed from the path, it will work without issue in Mask/Rust. Only had to do this one time and it was done via Powershell.

Edit: one more thing worth mentioning is that I am not the most experienced powershell dev. I tried to make all of the blocks consistent but there are a few items that might not be completely idiomatic.

@jacobdeichert
Copy link
Owner

@tensor-programming thanks so much for the PR!

I'll be able to review it this week, only peaked so far. Adding Windows CI is amazing 👍

@tensor-programming
Copy link
Contributor Author

Not a problem.

Probably going to refactor some of the Powershell stuff in the tests just to make it a little more consistent.

Copy link
Owner

@jacobdeichert jacobdeichert left a comment

Choose a reason for hiding this comment

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

Great PR! Such minimal changes to get full Windows support 😄

Left a few comment questions. After they're answered, I'll approve, merge and release 👍

.github/workflows/ci.yml Show resolved Hide resolved
maskfile.md Show resolved Hide resolved
maskfile.md Show resolved Hide resolved
maskfile.md Show resolved Hide resolved
maskfile.md Show resolved Hide resolved
src/executor.rs Show resolved Hide resolved
src/parser.rs Show resolved Hide resolved
tests/supported_runtimes_test.rs Show resolved Hide resolved
tests/env_vars_test.rs Show resolved Hide resolved
@tensor-programming
Copy link
Contributor Author

Alright, everything should be good to go.

@jacobdeichert
Copy link
Owner

@tensor-programming thanks! Just one more question ^^ and then we'll get this merged and deployed 👍

@tensor-programming
Copy link
Contributor Author

@tensor-programming thanks! Just one more question ^^ and then we'll get this merged and deployed 👍

Resolved it. Stemmed from an unnecessary misunderstanding of how mask and powershell dealt with shortcase flags.

@jacobdeichert
Copy link
Owner

@tensor-programming thanks! Yeah, that's why I was confused since mask only passes in the long form name :)

I'll try to merge and deploy tonight or tomorrow night 👍

@jacobdeichert jacobdeichert merged commit a849cc7 into jacobdeichert:master Apr 4, 2020
@jacobdeichert
Copy link
Owner

Ouff, i'm going to have to revert the merge for a bit. On macOS, mask is now broken and throwing this error:

ERROR: No such file or directory (os error 2)

@jacobdeichert
Copy link
Owner

jacobdeichert commented Apr 4, 2020

@tensor-programming when you have a min, can you make a new PR from your branch to master, just like this. I apparently can't reopen a merged PR 😓

I'll debug and figure out the macOS issue introduced by this 👍

Planning to get this re-merged and published this weekend 🤞

@tensor-programming tensor-programming deleted the feat/windows-powershell-support branch April 9, 2020 08:17
@tensor-programming tensor-programming restored the feat/windows-powershell-support branch April 9, 2020 08:18
@tensor-programming
Copy link
Contributor Author

tensor-programming commented Apr 9, 2020

Sorry, I just restored the branch and added a new PR.

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.

Windows support?
2 participants