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

Allow missing .py files if .pyc is present #1714

Merged
merged 4 commits into from Mar 13, 2020
Merged

Conversation

tucked
Copy link
Contributor

@tucked tucked commented Mar 11, 2020

Python can run with just .pyc files; however, if only a .pyc file is present (e.g. os.pyc), virtualenv will throw a RuntimeError: No virtualenv implementation for PythonInfo(...).

This makes it so the .py file is checked if it exists or if neither it nor the .pyc file exists (so as to preserve the current behavior when both are missing).

Thanks for contributing, make sure you address all the checklists (for details on how see

development documentation)!

  • ran the linter to address style issues (tox -e fix_lint)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 11, 2020

Can you describe a use case for this? Why would anyone want this?

@tucked
Copy link
Contributor Author

tucked commented Mar 11, 2020

Can you describe a use case for this? Why would anyone want this?

Custom systems may want to maximize available storage or obfuscate source code by removing .py files.

@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 11, 2020

Did this worked with old virtualenv or would this be a new feature here 🤔

@tucked
Copy link
Contributor Author

tucked commented Mar 11, 2020

Did this worked with old virtualenv or would this be a new feature here 🤔

I cannot reproduce the issue with virtualenv<20.

@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 11, 2020

I don't think this is an issue but feature 🤔 did it work to create virtual environments with virtualenv 16.x when you did not have the source file? I don't think so.

@tucked
Copy link
Contributor Author

tucked commented Mar 11, 2020

I don't think this is an issue but feature 🤔 did it work to create virtual environments with virtualenv 16.x when you did not have the source file? I don't think so.

Yes, if I downgrade to virtualenv<20 on the same system I repro'd this on, I can create a virtualenv.

@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 11, 2020

I'm surprised. Can you post a tree view of the result with old virtualenv and the output of -vvv 😲

@tucked
Copy link
Contributor Author

tucked commented Mar 11, 2020

I'm surprised. Can you post a tree view of the result with old virtualenv and the output of -vvv 😲

How's this (with virtualenv-16.7.10)?

@tucked
Copy link
Contributor Author

tucked commented Mar 11, 2020

I'm having a tough time making a test for this because writes to Path(interpreter.system_stdlib) are usually restricted 🤔

diff --git a/tests/unit/create/test_creator.py b/tests/unit/create/test_creator.py
index 70e0e6a..4091689 100644
--- a/tests/unit/create/test_creator.py
+++ b/tests/unit/create/test_creator.py
@@ -19,6 +19,7 @@ import pytest

 from virtualenv.__main__ import run, run_with_catch
 from virtualenv.create.creator import DEBUG_SCRIPT, Creator, get_env_debug_info
+from virtualenv.create.via_global_ref.builtin.cpython.cpython2 import CPython2
 from virtualenv.discovery.builtin import get_interpreter
 from virtualenv.discovery.py_info import PythonInfo
 from virtualenv.info import IS_PYPY, IS_WIN, PY3, fs_is_case_sensitive, fs_supports_symlink
@@ -448,3 +449,23 @@ def test_python_path(monkeypatch, tmp_path, python_path_on):
         assert non_python_path == [i for i in base if i not in extra_as_python_path]
     else:
         assert base == extra_all
+
+
+@pytest.fixture
+def creator_patched_modules(monkeypatch, current_creators):
+    creator = current_creators[2]
+    module_name = "CPython2_patch_modules_module"
+    module_path = Path(CURRENT.system_stdlib) / "{}.pyc".format(module_name)
+    module_path.touch()
+    @classmethod
+    def modules(cls):
+        return [module_name]
+    monkeypatch.setattr(CPython2, 'modules', modules)
+    yield creator
+    module_path.unlink()
+
+
+@pytest.mark.skipif(PY3, reason=".py files are only checked for in py2.")
+def test_pyc_only(creator_patched_modules):
+    assert creator_patched_modules.can_create(CURRENT) is not None
tests/unit/create/test_creator.py:459:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.tox/py27/lib/python2.7/site-packages/pathlib2/__init__.py:1524: in touch
    fd = self._raw_open(flags, mode)
.tox/py27/lib/python2.7/site-packages/pathlib2/__init__.py:1305: in _raw_open
    return self._accessor.open(self, flags, mode)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

pathobj = PosixPath('/usr/lib/python2.7/CPython2_patch_modules_module.pyc'), args = (65, 438)

    @functools.wraps(strfunc)
    def wrapped(pathobj, *args):
>       return strfunc(str(pathobj), *args)
E       OSError: [Errno 13] Permission denied: '/usr/lib/python2.7/CPython2_patch_modules_module.pyc'

args       = (65, 438)
pathobj    = PosixPath('/usr/lib/python2.7/CPython2_patch_modules_module.pyc')
strfunc    = <built-in function open>

.tox/py27/lib/python2.7/site-packages/pathlib2/__init__.py:585: OSError

@tucked tucked marked this pull request as ready for review Mar 11, 2020
@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 11, 2020

As a test I'd recommend building a mock python by copying the system files without the py files and running on that 👍

@ngie-eign
Copy link

ngie-eign commented Mar 12, 2020

Dumb question: why isn't this logic (before/after) using importlib, __import__, or something similar with a stripped down sys.path, then scrutinizing paths for modules it finds?

By the way, the current code is technically code broken with .pyo-only files, as well as compressed eggs. There are some other custom formats like .par and .xar, that might not work with this either.

I think it makes a lot more sense to rely on the python interpreter when it comes to import logic.

@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 12, 2020

It's because the python executing the code here is not the same as the target python. Importing it here would not give you the modules you're expecting. Those modes are not supported by virtualenv, and would require case by case judgment if we should support them or not.

@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 12, 2020

I'm fairly sure compressed eggs are not supported by the ecosystem, e.g. setuptools anymore. They work for backwards compatibility reasons but they should not be used.

@tucked
Copy link
Contributor Author

tucked commented Mar 12, 2020

As a test I'd recommend building a mock python by copying the system files without the py files and running on that 👍

I'm not entirely sure how to do this 😅 I was able to get something working, though.
I believe the CI failures are unrelated. (Note: test_pyc_only passed in the timed out pypy macOs run.)

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 13, 2020

@tucked see 9742921 for a more accurate test

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@tucked
Copy link
Contributor Author

tucked commented Mar 13, 2020

@tucked see 9742921 for a more accurate test

Awesome 😎 Thanks for that!
Looks like we're almost there... At least one of the CI failures looks relevant:

>       assert "os.pyc" in result.creator.debug["os"]
E       KeyError: u'os'

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 13, 2020

Yeah, pushed a fixed commit just now, seems PyPy does not support not having the source file for a standard library module.

@gaborbernat gaborbernat merged commit f1663de into pypa:master Mar 13, 2020
37 checks passed
@tucked tucked deleted the pyc branch Mar 13, 2020
@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 13, 2020

I'll do a release tomorrow 👍

@gaborbernat
Copy link
Contributor

gaborbernat commented Mar 19, 2020

Hello, a fix for this issue has been released via virtualenv 20.0.11; see https://pypi.org/project/virtualenv/20.0.11/ (https://virtualenv.pypa.io/en/latest/changelog.html#v20-0-11-2020-03-18). Please give a try and report back if your issue has not been addressed; if not, please comment here, and we'll reopen the ticket. We want to apologize for the inconvenience this has caused you and say thanks for having patience while we resolve the unexpected bugs with this new major release.
thanks

@tucked
Copy link
Contributor Author

tucked commented Mar 19, 2020

@gaborbernat Thank YOU for all your help with this 😊 No worries

I suspect this is fixed; however, I'm unable to verify due to #1738!

edit: Suspicion confirmed: 20.0.13 seems to work! Thanks again 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants