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 powershell shebang, closes #825 #826

Merged
merged 10 commits into from
May 11, 2021
Merged

Fix powershell shebang, closes #825 #826

merged 10 commits into from
May 11, 2021

Conversation

sigoden
Copy link
Contributor

@sigoden sigoden commented May 11, 2021

No description provided.

@casey
Copy link
Owner

casey commented May 11, 2021

Thanks for the PR!

Can you add a test that this works on windows? Check out the tests directory. You can put it in a new module, maybe tests/shebang.rs.

Also, what if the users writes:

foo:
  #!powershell

…without .exe. If that works, we should check for that too. Also, do we need to add a suffix for pwsh.exe?

Even though this is windows specific, let's do it on every platform. The less platform-specific code there is the better, and it won't hurt to add a .ps1 suffix on macOS and Linux.

src/platform_interface.rs Outdated Show resolved Hide resolved
src/platform_interface.rs Outdated Show resolved Hide resolved
src/recipe.rs Outdated Show resolved Hide resolved
tests/shebang.rs Outdated Show resolved Hide resolved
@casey casey enabled auto-merge (squash) May 11, 2021 08:04
src/recipe.rs Outdated Show resolved Hide resolved
tests/shebang.rs Outdated Show resolved Hide resolved
src/recipe.rs Outdated
@@ -115,7 +127,8 @@ impl<'src, D> Recipe<'src, D> {
io_error: error,
})?;
let mut path = tmp.path().to_path_buf();
path.push(self.name());
let suffix = Platform::get_script_file_suffix(interpreter);
Copy link
Owner

Choose a reason for hiding this comment

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

Since this isn't platform specific, we can just inline it:

Suggested change
let suffix = Platform::get_script_file_suffix(interpreter);
let suffix = if command.ends_with("powershell")
|| command.ends_with("powershell.exe")
{
".ps1"
} else {
""
};

@casey
Copy link
Owner

casey commented May 11, 2021

I made an additional suggestion. We can move the suffix function out of Platform because it's not platform specific anymore. Also, CI is catching some unformatted code. You can do cargo +nightly fmt --all to format the code. (You'll need nightly rust, since there's some additional rustfmt options that I enable that aren't available on stable.)

auto-merge was automatically disabled May 11, 2021 09:06

Head branch was pushed to by a user without write access

@casey casey merged commit cba52c9 into casey:master May 11, 2021
@casey
Copy link
Owner

casey commented May 11, 2021

Thanks again for the PR! Let me know if you'd like me to cut a release of Just that contains the new code.

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.

None yet

2 participants