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

prevent PermissionError when using venv creator on some systems #2543

Merged
merged 5 commits into from
Apr 27, 2023

Conversation

kulikjak
Copy link
Contributor

Fixes #2419

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Test and changelog too please.

@@ -41,6 +41,10 @@ def _generate(self, replacements, templates, to_folder, creator):
for template in templates:
text = self.instantiate_template(replacements, template, creator)
dest = to_folder / self.as_name(template)
# remove the file if it already exists - this prevents permission
# errors when the dest is not writeable
Copy link
Contributor

Choose a reason for hiding this comment

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

we can only remove if the parent directory is write-able, no? why would this file be not write-able and the parent be so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The venv creator specifically uses shutil.copymode on activate* files only (here). Other files in the directory (or the directory itself) are not affected.

> python3.7 -m virtualenv --creator venv venv
PermissionError: [Errno 13] Permission denied: '/..../venv/bin/activate'
> ls -la venv/bin/
drwxr-xr-x   2 user   group       16 Apr 14 15:51 .
drwxr-xr-x   5 user   group        8 Apr 14 15:51 ..
-r--r--r--   1 user   group     2207 Apr 14 15:51 activate
-r--r--r--   1 user   group     1259 Apr 14 15:51 activate.csh
-r--r--r--   1 user   group     2411 Apr 14 15:51 activate.fish
-rwxr-xr-x   1 user   group      243 Apr 14 15:51 pip
-rwxr-xr-x   1 user   group      243 Apr 14 15:51 pip-3.7
-rwxr-xr-x   1 user   group      243 Apr 14 15:51 pip3
-rwxr-xr-x   1 user   group      243 Apr 14 15:51 pip3.7
lrwxrwxrwx   1 user   group        9 Apr 14 15:51 python -> python3.7
lrwxrwxrwx   1 user   group        9 Apr 14 15:51 python3 -> python3.7
lrwxrwxrwx   1 user   group       14 Apr 14 15:51 python3.7 -> /bin/python3.7
-rwxr-xr-x   1 user   group      230 Apr 14 15:51 wheel
-rwxr-xr-x   1 user   group      230 Apr 14 15:51 wheel-3.7
-rwxr-xr-x   1 user   group      230 Apr 14 15:51 wheel3
-rwxr-xr-x   1 user   group      230 Apr 14 15:51 wheel3.7

@kulikjak
Copy link
Contributor Author

Test and changelog too please.

Do you mean test that this issue is no longer present? On our system (where installed files are not writeable by the owner by default) we saw several failures before, and I don't see those now:

FAILED tests/unit/create/via_global_ref/test_build_c_ext.py::test_can_build_c_extensions[venv]
FAILED tests/unit/create/test_creator.py::test_create_clear_resets[clear-venv]
FAILED tests/unit/create/test_creator.py::test_prompt_set[magic-venv]
FAILED tests/unit/create/test_creator.py::test_home_path_is_exe_parent[venv]
FAILED tests/unit/create/test_creator.py::test_prompt_set[None-venv]

But I will try to think about a test that you might "use" as well.

@gaborbernat
Copy link
Contributor

Can you replicate those failing tests within the CI and add a test for that? Also the pre-commit.ci is failin.g

@chrysle
Copy link
Contributor

chrysle commented Apr 21, 2023

Also the pre-commit.ci is failin.g

@gaborbernat Maybe you can add writeable to the whitelist?

@gaborbernat
Copy link
Contributor

That's up to the PR creator 👍

@kulikjak
Copy link
Contributor Author

kulikjak commented Apr 25, 2023

Also the pre-commit.ci is failin.g

@gaborbernat Maybe you can add writeable to the whitelist?

Sure, I can add it.

EDIT: it seems that writable is already being used in the codebase and writeable is not, so I changed it.

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Still needs test that would fail the CI without this fix.

@kulikjak
Copy link
Contributor Author

I am looking into that.

@kulikjak
Copy link
Contributor Author

I added a test that fails on Linux without the rest of this change (inspired by how test_value_alias does the mocking). Let's see how other OSes in the CI react to it.

@kulikjak
Copy link
Contributor Author

Hmmm, one test failed but I don't think that was caused by this change.

@gaborbernat gaborbernat merged commit 0597a2f into pypa:main Apr 27, 2023
37 checks passed
hroncok pushed a commit to fedora-python/virtualenv that referenced this pull request Apr 28, 2023
tarpas pushed a commit to tarpas/virtualenv that referenced this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PermissionError when using venv creator on Solaris
3 participants