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

Provide configuration options and saner defaults for shell kind to be used #15

Closed
JosXa opened this issue Nov 25, 2020 · 16 comments
Closed
Labels
enhancement New feature or request

Comments

@JosXa
Copy link

JosXa commented Nov 25, 2020

Original issue title: Poe tries to enforce WSL on Windows 10

When running a poe shell-type task in Windows Powershell Core 7.0.3, it seems to attempt execution of the command inside the Windows Subsystem for Linux (WSL), but I am happy using the cross-platform Powershell for most use cases.

Using the following configuration:

[tool.poe.tasks]
test = { shell = 'test' }

It complains that the WSL is not installed:

PS C:\projects\someproject> poe test
Poe => test
Windows Subsystem for Linux has no installed distributions.
Distributions can be installed by visiting the Microsoft Store:
https://aka.ms/wslstore
@nat-n
Copy link
Owner

nat-n commented Nov 25, 2020

I don't know much about windows dev environments, so I made a best guess about how to get POSIX shell scripts to work seamlessly on windows with a bit of trial and error, but not very exhaustive testing, and I may have missed something obvious.

The intention of the shell task type is POSIX shell scripts, so the canonical implementation is to pipe the content into a bash process. Therefore the strategy is to try find a bash executable to use by looking on the PATH, looking for git bash, or finally looking for WSL. It's all in this file: https://github.com/nat-n/poethepoet/blob/master/poethepoet/task/shell.py#L47

@JosXa What would you suggest doing differently?

@JosXa
Copy link
Author

JosXa commented Dec 4, 2020

Ah yes, that sounds perfect and actually looks like you've given it a lot of thought. I can also confirm that it works out of the box when WSL is installed. So then I'd argue we need an additional way to specify which shell we'd like to run on? The cmd also seems to be using the native Windows cmd.exe instead of the active powershell 🤔
I'd appreciate if it just used whatever I'm currently in for { cmd = ...}

@JosXa
Copy link
Author

JosXa commented Dec 5, 2020

So one workaround I found was the following:

testps = {shell = "pwsh.exe -ExecutionPolicy Bypass -Command \"cd .. && ls\""}

And works a little better with multiline strings:

update = { shell = """
pwsh.exe -ExecutionPolicy Bypass -Command "
    cd ../tgtypes && poetry update
    cd ../autogram-commons && poetry update
    cd ../botkit && poetry update
    poetry update
"
"""}
envinfo = { shell = """
pwsh.exe -ExecutionPolicy Bypass -Command "
    cd ../tgtypes && poetry env info -p &&
    cd ../autogram-commons && poetry env info -p &&
    cd ../botkit && poetry env info -p &&
    poetry env info -p
"
""" }

No idea who's now running that or how it does it, but it works 😆 Would still be nice to have high-level control and some more insight into what exactly is running my command here.

@nat-n
Copy link
Owner

nat-n commented Dec 5, 2020

So then I'd argue we need an additional way to specify which shell we'd like to run on?

I've considered making it possible to specify the default shell as an option, either globally or per task, but I'm not sure if this is better or potentially worse for the goal of tasks being universal/portable by default. Of course this goal is only sort of achievable in the first place, but adding support for users to choose a shell that might be less universal than bash is possibly a step backwards. On that note, it may be relevant to your use case that if the SHELL environment variable is set (as would normally be the case within a shell on linux/macOS) then it will be used to find the shell, which might not be bash (this might be a bad idea coming to think of it concerning users of fish for example).

The cmd also seems to be using the native Windows cmd.exe instead of the active powershell

My windows knowledge is weak, so I'm not sure what to make of this. The cmd task type is basically just a popen with the given content. Would it make sense for it to be something different? Would using Popen with shell=True help?

I'm glad you found a workaround, would explicitly calling pwsh.exe like that work from a cmd task instead of shell? It might be more performant.

If what you want is to write powershell specific tasks (i.e. that will never run on linux) then it would be trivial to create a new task type for this, which I hope to make well supported at some point #13

Would still be nice to have high-level control and some more insight into what exactly is running my command here.

What would you suggest for this? I've thought a bit about describing more about the task/execution when the global -v flag is given.

@JosXa
Copy link
Author

JosXa commented Dec 5, 2020

Hmm, wouldn't the safest, most reproducible way be to specify a custom shell command that actually points into a docker container?

If what you want is to write powershell specific tasks (i.e. that will never run on linux)

I do not necessarily, mainly I appreciate having the freedom to choose. And I may have to correct you on that statement, the versions 7.+ of Powershell Core are named that way because they are based on the .NET Core runtime (which is superbly linux friendly), meaning that more and more linux devs are starting to pick it up aswell.
I'm not a mega fan or anything, it's just what comes to me most convenient on a Windows host (from work). My personal alternative would be to spend at least 3 hours trying to figure out how I can get consistent (read: indentical .exe paths) dev environments between WSL and Windows, and that should be possible but also not something you wanna push new adopters to do immediately.

I do like the shell command type a lot, but it's only valuable right now if it's accomodated by a working cmd task. Unfortunately I've actually never spawned a powershell from python, so that's what I'm gonna try tomorrow.

Windows "cmd.exe" is the old, built-in default terminal that's mainly used as a task runner by people, but rarely for scripting. Afaik powershell.exe is the version that ships with windows, while it was renamed to pwsh.exe for the modern Core version. So you see, there's no one way fits all here...

A zsh or fish user might wanna run in the WSL, or some people may even try to play around with open ssh connections or whatever... Point is: The different types are cool, but there just needs to be some config for that, or the ability to run from current shell, IMO :)

@JosXa
Copy link
Author

JosXa commented Dec 5, 2020

Oh, and they added aliases to pwsh so basically I'm typing bash commands all the time and it just works the same

@TBBle
Copy link

TBBle commented Jan 4, 2021

Would it be clearer if the shell executor was named posix or perhaps sh? I would have assumed shell was "run in your shell", not "hunt around for bash, whatever environment it might be found it". The hunt-around behaviour is neat, but surprising if you don't assume "shell" means specifically "bourne(-again) shell", even on platforms where that is not the native shell.

It doesn't help that a shell task is different from poetry shell (always launches cmd.exe, even if I have $ENV:SHELL set in my environment... That's python-poetry/poetry#2030 though not a poethepoet issue).

@boukeversteegh
Copy link

boukeversteegh commented Jan 4, 2021

Would it be clearer if the shell executor was named posix or perhaps sh? I would have assumed shell was "run in your shell", not "hunt around for bash, whatever environment it might be found it". The hunt-around behaviour is neat, but surprising if you don't assume "shell" means specifically "bourne(-again) shell", even on platforms where that is not the native shell.

It doesn't help that a shell task is different from poetry shell (always launches cmd.exe, even if I have $ENV:SHELL set in my environment... That's python-poetry/poetry#2030 though not a poethepoet issue).

That brings up an interesting idea, perhaps, to detect whether the containing process is bash or cmd, and run the shell with that same process.

I.e. if you want poe to use bash, execute it within a bash environment. If you are running it in cmd, it will use cmd.

The following snippet detects the grandfather process (parent is python.exe):

import psutil
print(psutil.Process().parent().parent().exe())

Returns either of:

C:\Program Files\Git\usr\bin\bash.exe
C:\Windows\System32\cmd.exe

It solves the problem of figuring out where the shell is located, and ensures that the shell actually exists / works, because poe is being run from it.

Also, Windows users who also use bash, will probably run poe from their git-bash or other bash environment anyway.

@TBBle
Copy link

TBBle commented Jan 4, 2021

For detecting the shell, Poetry itself, and pipenv, use shellingham which wraps up such psutil-style logic nicely.

That said, I'm not fond of this idea, as it's very unlikely shell script will be written in a way that works with any of bash, zsh, fish, cmd, PowerShell or pwsh. shell tasks are different from poetry shell or pipenv shell (another reason to rename it), because in the latter cases, it's the user who is writing commands in the shell, and should get the shell they expect. In the task case, it's the script author who is writing commands in the shell, and should get the shell they expect.

@nat-n
Copy link
Owner

nat-n commented Jan 10, 2021

@JosXa @boukeversteegh @TBBle Thanks for the input.
 
The idea of just using the user's current shell (across POSIX & windows) as a default behaviour seems quite problematic to me, since I would expect mac/linux users to frequently employ shell features or syntax that won't work on cmd or pwsh, thus inadvertently marginalising collaborators who prefer a windows environment, and vice versa. This is arguably already a problem, since there's no guarantee that a suitable POSIX shell is available, but at least the failure mode is simple.
 
Therefore considering the goal of making tasks easy and convenient to write in as predictable and portable a way as possible, I'm not sure if there's a better default than bash or bust. Don't get me wrong, I'm still open to considering alternatives or ideas for improving the implementation, but I'm not yet sure what that would look like.
 
That said improving support for other use cases (like pwsh) with better configurability is definitely something I want to do. So please consider the following proposal/"Request For Comments":
 

  1. Shell tasks should respect a task level or global option for specifying an ordered list of shells to try to use, with a default of ["bash", "sh"].
  2. Items in the list should probably refer to shells by name, so there can be hard coded logic for how to find and invoke each shell type. Maybe it could allow user's to provide an arbitrary shell path and guess from the name how to invoke it, but I'm not sure that's a good idea.
  3. This could include be an "inherit" option, that uses whatever shell the user is in, in case that's useful.

As an aside: since unlike pipenv/poetry shell this isn't about giving the user an interactive shell to play in, I don't think it's too important to support every shell flavour a user might like to use; really just that a few lines of "shell script" behave as expected.
 
Would this support your use cases well? Do you see anything missing or problematic with this?



I would need some help from a windows native developer to implement the appropriate windows shell support.

@nat-n nat-n added the enhancement New feature or request label Jan 10, 2021
@boukeversteegh
Copy link

Yes, you are right, just blindly running the commands in whatever shell is active is not a good idea.

But combined with your idea of a compatibility list (with global default) could perhaps make it work smoothly.

Suppose:

[tool.poe.tasks]
foo = { shell = 'test', shells = {'bash', 'zsh', 'sh'} }

User then tries to run in the wrong shell, and then corrects it.

C:\project> poe run foo
[poe] error: you cannot run task [foo] in [command.exe]. Please execute this script from: bash / zsh / sh.

C:\project> wsl.exe

Welcome to Ubuntu!

/c/project $ echo $SHELL
/bin/bash # proving that we are running bash now

/c/project $ poe run foo
[poe] task foo completed

This approach could make the following challenges easier to deal with:

@TBBle
Copy link

TBBle commented Jan 11, 2021

I'm not sure it's a delightful workflow to make the shell-type tasks require the user to already be in the current shell.

I'd suggest that apart from bash.exe on Windows being a WSL-launcher, "Try and run the named shell (currently implicitly bash)" is reasonable, as if I'm writing scripts in a certain shell, I'm placing the onus on my user to have that shell available -- in my use-case, it's a corporate environment, so it's feasible for our system to require "Has bash installed" or "Has pwsh installed".

So perhaps separately from anything else, blacklisting C:\\Windows\\bash.exe as a match for bash would keep this consistent, and the shells optional table can address the general use-case of shell-type scripts can be written in languages other than 'bash'.

As another example of a shell task, to motivate my current thinking that the shell type of task is similar to a script with a #! first-line, telling the system what interpreter to use.

[tool.poe.tasks."print-python-ver"]
shells={'python'}
shell="""
import sys
print(sys.version)
"""

@nat-n
Copy link
Owner

nat-n commented Jan 11, 2021

I'm not sure it's a delightful workflow to make the shell-type tasks require the user to already be in the current shell.

I'm inclined to agree with this. Though maybe using the user's current shell as a hint when it's time to go hunting is a good idea.

blacklisting C:\Windows\bash.exe as a match for bash would keep this consistent

Could expand upon your thinking here? Do you mean to say that the current fallback to invoking wsl bash makes no sense unless the user is already in wsl bash?

I've so far resisted the temptation to support inline python scripts because writing python programs inside toml doesn't seem like a good practice to encourage. Though the way you put it there as equivalent to a shebang is interesting.

@TBBle
Copy link

TBBle commented Jan 12, 2021

Yeah, I think if the user is in WSL bash already, they're in a POSIX isolated environment, and will have real bash in their PATH, and it'll all work fine. They're not really working in Windows, so all this becomes academic.

If they're not in WSL, it seems improbable to me that executing WSL Bash would actually come up with the same functional environment, in terms of "Things available in the path", that other Windows-based Bash environments provide. For example, despite using the Poetry executor, the Poetry venv would no longer be active, and in fact Poetry itself stops being available ($PATH doesn't carry across), testing from a poetry shell session. In fact, my WSL default instance (Ubuntu) doesn't have python in the path (it's python3) as is typical of POSIX, and never the case in Windows.

So the only way WSL bash makes sense is if ssh <some random linux box> "mount -t cifs <where I came from> && bash" made sense, i.e. your files are there, but the rest of your environment is missing, including (I suspect but haven't tested) the environment variables set by the poethepoet task definition.

Both Git Bash and MSYS2 Bash seem to remain functional when launched from a poetry shell session, including keep the virtual-env activated.

So if the users seeing this have a mix of Git/MSYS2 Bash available and not-available, some are going to be in the expected state, and some are going to be effectively working from within a unitialised environment, so non-trivial bash scripts are going to be surprisingly broken, I expect.

@JosXa JosXa changed the title Poe tries to enforce WSL on Windows 10 Provide configuration options and saner defaults for shell kind to be used Jan 12, 2021
@nat-n
Copy link
Owner

nat-n commented Dec 21, 2021

@JosXa @TBBle @boukeversteegh

It only took a year to get around to picking this up again (that's side projects for you), but I've implemented something that I hope works for everyone.

With the change in PR you can configure a single shell (by name) or an ordered list of shells to try, either per task by specifying the interpreter task option, or globally by specifying the shell_interpreter option. The set of recognised interpreters is {"sh", "bash", "zsh", "pwsh", "python"}. The default is "posix" which is an alias for ["sh", "bash", "zsh"].

Note that there are a couple of significant limitations with the powershell integration that I don't know how to solve. Firstly when powershell starts it always prints some copyright boilerplate which is not really desirable for this use case, and secondly it doesn't propagate environment variables from the parent process which is quite a limitation (e.g. named args won't work). Suggestions are welcome from anyone more knowledgable about powershell.

Please have a look at the PR, I'll leave it open for a little while in case there's some feedback, but of course it's worth pointing out potential issues even after it's merged.

@nat-n
Copy link
Owner

nat-n commented Jan 8, 2022

A fix for this issue has just been released as 0.12.0

@nat-n nat-n closed this as completed Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants