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

python.poetryPath default setting isn't working on Windows #9672

Closed
nicolaerario opened this issue Jan 20, 2020 · 10 comments · Fixed by #15707
Closed

python.poetryPath default setting isn't working on Windows #9672

nicolaerario opened this issue Jan 20, 2020 · 10 comments · Fixed by #15707
Assignees
Labels
area-environments Features relating to handling interpreter environments bug Issue identified by VS Code Team member as probable bug

Comments

@nicolaerario
Copy link

Environment data

  • VS Code version: 1.14.1
  • Extension version (available under the Extensions sidebar): 2020.1.58038
  • OS and version: Windows 10
  • Python version (& distribution if applicable, e.g. Anaconda): 3.8.1
  • Type of virtual environment used (N/A | venv | virtualenv | conda | ...): in project .venv
  • Relevant/affected Python packages and their versions: poetry v1.0.2 'in the path'
  • Jedi or Language Server? (i.e. what is "python.jediEnabled" set to; more info How to update the language server to the latest stable version #3977): both

Expected behaviour

poetry add --dev pylint

Actual behaviour

python -m pip install pylint

Steps to reproduce:

  1. poetry init
  2. poetry install
  3. create a python file
  4. the python extension start and activate the virtualenv as expected
  5. popup that inform no linter found in venv, ask to install
  6. this will install linter without poetry

The default python.poetryPath is set to poetry and this is fine on linux;
on windows however, though the executable is in the path and is actually poetry, it is called by the poetry.bat file
this means that the right python.poetryPath for windows had to be set to poetry.bat

This isn't really a big issue, maybe can only be explicit as a settings reference

@nicolaerario nicolaerario added triage-needed Needs assignment to the proper sub-team bug Issue identified by VS Code Team member as probable bug labels Jan 20, 2020
@karthiknadig karthiknadig self-assigned this Jan 21, 2020
@ghost ghost removed the triage-needed Needs assignment to the proper sub-team label Jan 21, 2020
@karthiknadig
Copy link
Member

Looks like there are two issues here.

  1. poetryPath should point to poetry.bat on windows. but points to poetry.
  2. when we attempt to install a library we are not using poetry.

@nicolaerario Can you create a separate issue for the library installation? this would be easier for tracking.

@karthiknadig karthiknadig added area-environments Features relating to handling interpreter environments needs PR and removed triage labels Jan 22, 2020
@nicolaerario
Copy link
Author

Sure I can do it, but it is really needed? I mean that, if poetryPath points to poetry.bat file, libraries installation is fine.

@karthiknadig
Copy link
Member

When you explicitly set the python.poetryPath to poetry.bat, does it install using poetry.bat add --dev pylint or poetry.bat -m pip install pylint?

@nicolaerario
Copy link
Author

nicolaerario commented Jan 22, 2020

The terminal pass poetry.bat add <module> as expected (poetry.bat add --dev <module> also is passed when needed). So poetry is invoked as expected on every need, no pip is used, simply by settingpython.poetryPath to poetry.bat.

Edit: corrected phone text formatting issue

@DonJayamanne
Copy link

Curious, why do we have to use poetry.bat? Is this the recommendation from the poetry site for Windows users?
If this is the case, then @karthiknadig we'll need to change the description in the settings and change the code to support *.bat files in poetry.
I.e. when we check whether poetry exists, we run <poetry path> --version. However that might not work. I guess that's where the issue is (that doesn't work, hence extension things poetry doesn't exist and hence doesn't use poetry to install the dependencies).

I'd check the recommended approach for running poetry on windows. If there's an executable, then we should use the executable in the setting else our code needs to change to support *.bat and that could be a bigger change. I.e. anywhere we use it, we'll need to ensure we run the bat file using the cmd executable or in a terminal. I.e. an exception for the poetry tool. This is something I'd hope we could avoid.

@karthiknadig
Copy link
Member

karthiknadig commented Jan 24, 2020

On windows they have both the poetry executable (without .exe extension) and poetry.bat in the %USERPROFILE%/.poetry/bin directory:
image

The extension picked up the right thing, which is the poetry executable. but it used the wrong command to install.

@nicolaerario
Copy link
Author

Just to add some details:
With integrated terminal (powershell), manually use of poetry commands (without bat ext) works normally: so poetry is installed right and fully functional and I can use it system wide and inside vscode also; so the manual workflow of poetry init, poetry install, poetry add works as expected in vscode.
Thinking can be related to setting not really point to the poetry executable, I tried to explicit set the executable absolute path; this not resolve the issue: when vscode ask for linter (for example), install it with pip; poetry don't know that pylint is in venv (toml and lock files aren't modified obliviusly). Manual poetry add black --dev works normally instead.
Set python.poetryPath to poetry.bat (relative path) make vscode to invoke poetry.bat add pylint --dev, not pip.
I know isn't elegant, but works

@karthiknadig karthiknadig removed their assignment Feb 26, 2020
@luabud luabud added needs PR needs proposal Need to make some design decisions and removed needs decision labels Feb 26, 2020
@luabud
Copy link
Member

luabud commented Feb 26, 2020

Maybe we shouldn't let poetryPath point to the bat file.

I think we could prompt an error saying:

"The path to poetry is invalid. Please point it to the executable file."

What do folks here think?

@nicolaerario
Copy link
Author

nicolaerario commented Feb 27, 2020

de facto pointing to the executable file (poetry in %USERPROFILE%/.poetry/bin) do nothing in windows and this is (surely I can be wrong ) the reason for the presence of the .bat file
EDIT: the path is irrilevant. both relative and absolute don't work. better: they works only if pointed to the .bat

@karrtikr karrtikr self-assigned this Mar 5, 2021
@karrtikr
Copy link

karrtikr commented Mar 9, 2021

Spike results

Due to an upstream Windows only bug on node nodejs/node-v0.x-archive#2318 (also see stack overflow answer), child_process.spawn fails to run files without any extension. poetry file inside %USERPROFILE%/.poetry/bin is not an executable, hence spawning it via child_process fails. It is documented shell: true may help us out there, but using it doesn't return any output for the command for some reason.

The easiest option is just to use child_process.exec instead of child_process.spawn to run poetry, which is provided via

public shellExec(command: string, options: ShellOptions = {}): Promise<ExecutionResult<string>> {

It works fine both for poetry and poetry.bat files.

@karrtikr karrtikr added needs PR and removed needs proposal Need to make some design decisions labels Mar 9, 2021
@ghost ghost removed the needs PR label Mar 19, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-environments Features relating to handling interpreter environments bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants