Skip to content

Comments

Resolve cwd using os.path.realpath() when generating environment names#6110

Merged
radoering merged 4 commits intopython-poetry:masterfrom
adamgreg:fix_win_mapped_drive_env
Aug 5, 2022
Merged

Resolve cwd using os.path.realpath() when generating environment names#6110
radoering merged 4 commits intopython-poetry:masterfrom
adamgreg:fix_win_mapped_drive_env

Conversation

@adamgreg
Copy link
Contributor

@adamgreg adamgreg commented Aug 3, 2022 •
edited
Loading

Pull Request Check List

Resolves: #6109

  • Added tests for changed code.
  • Updated documentation for changed code.

@adamgreg
Copy link
Contributor Author

adamgreg commented Aug 3, 2022

I wasn't sure how to add a suitable new unit test, sorry.

radoering
radoering requested changes Aug 3, 2022
View reviewed changes
@radoering
Copy link
Member

radoering commented Aug 3, 2022

I wasn't sure how to add a suitable new unit test, sorry.

You probably have to mock realpath().

@adamgreg
Copy link
Contributor Author

adamgreg commented Aug 3, 2022

I wasn't sure how to add a suitable new unit test, sorry.

You probably have to mock realpath().

realpath() returns its argument unchanged if the path does not exist, so I don't think it's necessary to mock it unless we want to count the calls.

I meant to say I can't think of a way to unit test that this actually fixes the bug, short of mapping a drive on the test system.

@radoering
Copy link
Member

radoering commented Aug 3, 2022

Let's say you have path1 and path2. You can mock realpath to return path1 for both (i.e. path2 is a symlink pointing to path1). Then, generate_env_name() should return the same value no matter if it's called with path1 or path2. A test case similar to

poetry/tests/utils/test_env.py

Lines 1356 to 1362 in 62701fe

def test_generate_env_name_ignores_case_for_case_insensitive_fs(tmp_dir: str):
venv_name1 = EnvManager.generate_env_name("simple-project", "MyDiR")
venv_name2 = EnvManager.generate_env_name("simple-project", "mYdIr")
if sys.platform == "win32":
assert venv_name1 == venv_name2
else:
assert venv_name1 != venv_name2
should be sufficient (just with mocking and without platform distinction).

Alternatively, instead of mocking you could create a symlink similar to

poetry/tests/packages/test_locker.py

Line 760 in 62701fe

def test_lock_file_resolves_file_url_symlinks(root: ProjectPackage):
although that's probably a bit more overhead.

@adamgreg adamgreg force-pushed the fix_win_mapped_drive_env branch from c8c6129 to 1d4321a Compare August 4, 2022 16:56
@adamgreg adamgreg requested a review from radoering August 4, 2022 16:57
@radoering
Copy link
Member

radoering commented Aug 5, 2022

pre-commit.ci autofix

@radoering radoering merged commit 601dc78 into python-poetry:master Aug 5, 2022
@finswimmer finswimmer mentioned this pull request Aug 16, 2022
Merged
@neersighted neersighted mentioned this pull request Sep 6, 2022
Closed
3 tasks
@github-actions
Copy link

github-actions bot commented Feb 29, 2024

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
@adamgreg adamgreg deleted the fix_win_mapped_drive_env branch February 29, 2024 10:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@radoering radoering radoering approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Windows drive mappings break virtual environment name

2 participants

@adamgreg @radoering