-
Notifications
You must be signed in to change notification settings - Fork 6
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
Use conda_package_streaming
for tarball extraction to avoid umask problems
#74
Conversation
tests/test_main.py
Outdated
process = subprocess.Popen( | ||
[CONDA_EXE, "constructor", "--prefix", tmp_path, "--extract-tarball"], | ||
stdin=subprocess.PIPE, | ||
umask=0o022, | ||
) | ||
process.communicate(tarbytes.getvalue()) | ||
rc = process.wait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could run_conda
be expanded to add stdin strings or is it not worth the effort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to send stdin in one more test, I'll do that :P
tests/test_main.py
Outdated
assert rc == 0 | ||
if sys.platform != "win32": | ||
mode = (tmp_path / "naughty_umask").stat().st_mode | ||
assert not mode & stat.S_IWGRP, "%o" % mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only checks group writing permissions. This is the origin of the bug, but shouldn't the entire umask be tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. I copied the logic from Daniel's PR but maybe I can simply test the resulting chmod. Should be 0o777-0o22
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New commit added, hopefully with the expected changes :)
Description
Should help with reports like conda/conda-libmamba-solver#522 and conda-forge/miniforge#506 (comment).
Checklist - did you ...
news
directory (using the template) for the next release's release notes?