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

Don't understand WSL behaviour #101

Open
sravioli opened this issue Jun 11, 2023 · 6 comments
Open

Don't understand WSL behaviour #101

sravioli opened this issue Jun 11, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@sravioli
Copy link

I have the following blocks in my maskfile:

# Tasks

> Useful tasks for the project

## install

> Will create a python virtual env and install the required dependencies.

Creates a virtual environment, activates it, upgrades pip to the latest version
and installs the required dependencies.

```bash
[[ "$verbose" == "true" ]] && echo "Creating python environment"
python3 -m venv .venv

[[ "$verbose" == "true" ]] && echo "Activating python environment"
source .venv/bin/activate

[[ "$verbose" == "true" ]] && echo "Upgrading pip"
python3 -m pip install --upgrade pip

[[ "$verbose" == "true" ]] && echo "Installing required dependencies"
pip install \
    mkdocs-git-revision-date-localized-plugin \
    pillow \
    cairosvg \
    mkdocs-glightbox \
```

```pwsh
if ($env:verbose) { Write-Output "Creating python environment" }
python -m venv .venv

if ($env:verbose) { Write-Output "Activating python environment" }
./.venv/Scripts/activate

if ($env:verbose) { Write-Output "Upgrading pip" }
python -m pip install --upgrade pip

if ($env:verbose) { Write-Output "Installing required dependencies" }
pip install `
    mkdocs-git-revision-date-localized-plugin `
    pillow `
    cairosvg `
    mkdocs-glightbox `
```

...

Calling mask install on powershell core, the script works, calling it inside Ubuntu (in WSL), i get the following message:

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

By commenting out the pwsh code block the script runs fine.

I installed pwsh on WSL just to test it, and this is what i get:

$ mask install
python:
Line |
   2 |  python -m venv .venv
     |  ~~~~~~
     | The term 'python' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
./.venv/Scripts/activate:
Line |
   5 |  ./.venv/Scripts/activate
     |  ~~~~~~~~~~~~~~~~~~~~~~~~
     | The term './.venv/Scripts/activate' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
python:
Line |
   8 |  python -m pip install --upgrade pip
     |  ~~~~~~
     | The term 'python' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
Defaulting to user installation because normal site-packages is not writeable
Collecting mkdocs-git-revision-date-localized-plugin
@sravioli
Copy link
Author

I changed pwsh to powershell and now it works fine in Ubuntu WSL. Why does mask behave this way with a pwsh code block?

@jacobdeichert
Copy link
Owner

Hey, thanks for reporting this.

Mask only checks for "powershell". Because it doesn't recognize "pwsh", it tries to fall back to assuming that command is on the system I think.

If "pwsh" is a common form of that lang code, we should add a check for it in mask.

@sravioli
Copy link
Author

If "pwsh" is a common form of that lang code, we should add a check for it in mask.

This I do not know.

PowerShell and pwsh are basically two different shells. PowerShell v5 comes preinstalled in Windows, while pwsh (or PowerShell-core or PowerShell v7) is cross platform and has to be installed manually.

If the user creates a pwsh code block, mask could check if the pwsh shell is installed on the system and execute the command, otherwise fallback to the system shell (bash/powershell)? Cold this work?

@jacobdeichert
Copy link
Owner

jacobdeichert commented Oct 5, 2023

Yeah we can probably add some way to skip code blocks if the system doesn't have the expected executor installed.

In general, I think that may lead to a bad user experience if we just silently ignore code blocks when the tool isn't installed. Some users would expect to see an error if the code block failed to execute.. new users probably want to know that they are missing a tool that the script expects.

So we should either:

  1. Add a mask flag (for example: --skip-missing-executors) which will silently ignore any code blocks where the system is missing the tool

  2. Add a code block lang code indicator (for example: ~~~pwsh(optional)) which would also silently ignore it if the executor is missing

  3. Or just succeed if at least one of the executors exists when there's multiple code blocks specified

I'll have to consider this a bit further, but so far 3 might be the simplest option.

@sravioli
Copy link
Author

sravioli commented Oct 5, 2023

Yeah we can probably add some way to skip code blocks if the system doesn't have the expected executor installed.

In general, I think that may lead to a bad user experience if we just silently ignore code blocks when the tool isn't installed. Some users would expect to see an error if the code block failed to execute.. new users probably want to know that they are missing a tool that the script expects.

So we should either:

  1. Add a mask flag (for example: --skip-missing-executors) which will silently ignore any code blocks where the system is missing the tool
  2. Add a code block lang code indicator (for example: ~~~pwsh(optional)) which would also silently ignore it if the executor is missing
  3. Or just succeed if at least one of the executors exists when there's multiple code blocks specified

I'll have to consider this a bit further, but so far 3 might be the simplest option.

Yup 3 looks easier. Maybe mask could also print that the executor is missing and that it's using the other one just to let the user know.

@jacobdeichert
Copy link
Owner

jacobdeichert commented Oct 6, 2023

Maybe mask could also print that the executor is missing and that it's using the other one just to let the user know.

The next release of mask will print a more helpful error when the executor is missing.

After looking at the code again, in your case you have 2 code blocks which is causing the parser to loop over both and only stores the last one - this is why WSL is trying to run pwsh because it's the final code block defined and we don't prevent that because we currently only check for powershell, batch, and cmd.

However, I have a solution similar to option 3 above but a little easier to reason about: if 2 code blocks are defined, macOS/linux will only check the first code block and expect that to work. And for windows, it'll use the final code block defined.

This means that:

  1. if a maskfile is only written with windows in mind, the first code block will still run as expected

  2. if a maskfile is written with cross-platform in mind, the first code block must be for macOS/linux and the second code block must be for windows

  3. the second code block for windows will no longer be limited to just powershell/cmd/batch scripts

As long as all maskfiles follow this rule, things should work as expected. In terms of breaking changes, it would currently be a bug if someone wrote 2 code blocks and was only targeting macOS/linux - the first block never runs. However, if they wrote 2 code blocks and they always write the first one for Windows (powershell, cmd, batch), then this would be a breaking change for these users. I think this is completely fine to release for 0.12.

So with that said, if anyone has time to take this, feel free. Otherwise I'll try to get to it at some point this year.

We should only need to update this file. Removing the lang code checks (powershell, cmd, batch) and instead counting code block iterations. First block always is for macOS/linux platform, and for windows we check for a second code block.

@jacobdeichert jacobdeichert added the enhancement New feature or request label Oct 6, 2023
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

2 participants