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: conda environment is detected as broken (#4566) #5007

Merged

Conversation

finswimmer
Copy link
Member

@finswimmer finswimmer commented Jan 8, 2022

conda puts the python and pip executable on windows in different location. So poetry is unable to detect a valid venv. This PR fixes this.

There is also a port for 1.1: #5008

Resolves: #4566

@zEdS15B3GCwq
Copy link

zEdS15B3GCwq commented Jan 11, 2022

Some ideas off the top of my head. Sorry if I'm wrong.

From the original env.py > Env > _bin code:

        if self._is_windows and not bin.endswith(".exe"):
            bin_path = self._bin_dir / (bin + ".exe")
        else:
            bin_path = self._bin_dir / bin

        if not bin_path.exists():
            # On Windows, some executables can be in the base path
            # This is especially true when installing Python with
            # the official installer, where python.exe will be at
            # the root of the env path.
            if self._is_windows:
                if not bin.endswith(".exe"):
                    bin_path = self._bin_dir / (bin + ".exe")  # <<< bug?
                else:
                    bin_path = self._path / bin

I have a feeling that the inside bin_path = self._bin_dir / (bin + ".exe") is a bug - it's already handled, it's known that it doesn't exist. Shouldn't it be bin_path = self._path / (bin + ".exe")? Also, self._path == self._bin_dir.parent, so a simple fix covers the same case as your patch:

        if not bin_path.exists():
            # On Windows, some executables can be in the base path
            # This is especially true when installing Python with
            # the official installer, where python.exe will be at
            # the root of the env path.
            if self._is_windows:
                if not bin.endswith(".exe"):
                    bin_path = self._path / (bin + ".exe")  # <<< fixed?
                else:
                    bin_path = self._path / bin

@zEdS15B3GCwq
Copy link

On the same note, in find_executables, perhaps

    def find_executables(self) -> None:
        if self._is_windows and self._is_conda:
            python_bin_dir = self._bin_dir.parent
        else:
            python_bin_dir = self._bin_dir

could be

    def find_executables(self) -> None:
        if self._is_windows and self._is_conda:
            python_bin_dir = self._path  # <<< this?
        else:
            python_bin_dir = self._bin_dir

@finswimmer
Copy link
Member Author

Thanks a lot for your feedback @zEdS15B3GCwq. 👍

I'm aware that there are some ugly parts that should be improved. That's why its a draft pr and I called it a proof-of-concept. Once I get some feedback in the corresponding issue whether it works as expected or not I will start improving the code.

fin swimmer

Copy link

@zEdS15B3GCwq zEdS15B3GCwq left a comment

Choose a reason for hiding this comment

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

sorry, ignore this

@finswimmer
Copy link
Member Author

sorry, ignore this

No, I will not. You've given some good points 😃

@zEdS15B3GCwq
Copy link

sorry, ignore this

No, I will not. You've given some good points 😃

That's nice of you, but I meant that particular message (or did you known already).

@finswimmer finswimmer force-pushed the issue-4566-broken-conda branch from 8fb4477 to 602722e Compare January 14, 2022 19:15
@finswimmer finswimmer marked this pull request as ready for review January 15, 2022 18:21
@finswimmer finswimmer requested a review from a team January 15, 2022 18:21
@finswimmer finswimmer changed the title add alternative bin search path in case of conda env under windows Fix: conda environment is detected as broken (#4566) Jan 15, 2022
@finswimmer finswimmer added the impact/backport Requires backport to stable branch label Jan 16, 2022
@neersighted neersighted merged commit 85ff214 into python-poetry:master Jan 17, 2022
@finswimmer finswimmer mentioned this pull request Mar 6, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
impact/backport Requires backport to stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poetry after 1.1.9 fails to recognize conda environments as valid
3 participants