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

Don't fail on missing pip/certifi's cacert.pem #1252

Merged
merged 2 commits into from Dec 6, 2018

Conversation

Projects
None yet
2 participants
@hroncok
Contributor

hroncok commented Dec 5, 2018

See https://github.com/pypa/pip/blob/master/src/pip/_vendor/README.rst

It is acceptable to remove the pip/_vendor/requests/cacert.pem file from pip

Virtualenv should not error on that

@gaborbernat

Let's add some tests for this. You can create e.g. a venv, remove it this file and see that we still pass.

@hroncok

This comment has been minimized.

Contributor

hroncok commented Dec 5, 2018

Sure, tests are great.

What needs to be done in tests:

  • create a new virtual environment
  • install virtualenv package into it
  • patch the pip whl from the virtualenv_support package not to have and use certifi's cacert.pem but instead use /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem
  • create a new virtual environment using that virtualenv

this currently blows with:

(__venv__) [virtualenvtest]$ python -m virtualenv test_virtualenv
Using base prefix '/usr'
New python executable in /home/churchyard/tmp/virtualenvtest/test_virtualenv/bin/python
Installing setuptools, pip, wheel...

  Complete output from command /home/churchyard/tmp...irtualenv/bin/python - setuptools pip wheel:
  Traceback (most recent call last):
  File "<stdin>", line 9, in <module>
  File "/usr/lib64/python3.7/pkgutil.py", line 637, in get_data
    return loader.get_data(resource_name)
OSError: [Errno 0] Error: 'pip/_vendor/certifi/cacert.pem'
----------------------------------------
...Installing setuptools, pip, wheel...done.
Traceback (most recent call last):
  File "/usr/lib64/python3.7/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib64/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/home/churchyard/tmp/virtualenvtest/__venv__/lib64/python3.7/site-packages/virtualenv.py", line 2462, in <module>
    main()
  File "/home/churchyard/tmp/virtualenvtest/__venv__/lib64/python3.7/site-packages/virtualenv.py", line 762, in main
    symlink=options.symlink,
  File "/home/churchyard/tmp/virtualenvtest/__venv__/lib64/python3.7/site-packages/virtualenv.py", line 1015, in create_environment
    install_wheel(to_install, py_executable, search_dirs, download=download)
  File "/home/churchyard/tmp/virtualenvtest/__venv__/lib64/python3.7/site-packages/virtualenv.py", line 968, in install_wheel
    call_subprocess(cmd, show_stdout=False, extra_env=env, stdin=SCRIPT)
  File "/home/churchyard/tmp/virtualenvtest/__venv__/lib64/python3.7/site-packages/virtualenv.py", line 854, in call_subprocess
    raise OSError("Command {} failed with error code {}".format(cmd_desc, proc.returncode))
OSError: Command /home/churchyard/tmp...irtualenv/bin/python - setuptools pip wheel failed with error code 1

With my patch, it should not.

Now if I provide Python code that does and verifies the above, would you be able to guide me on where to stick that into the current test suite?

@gaborbernat

This comment has been minimized.

Contributor

gaborbernat commented Dec 5, 2018

Put it at the end of https://github.com/pypa/virtualenv/blob/master/tests/test_virtualenv.py#L334; I would also caution you to not hardcode to etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem, but instead e.g. move pips into a temp folder and point to that. This way the test can succeed independent if etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem exists or not.

@hroncok hroncok force-pushed the hroncok:nopem branch from aa82b3b to cf9a681 Dec 6, 2018

@hroncok

This comment has been minimized.

Contributor

hroncok commented Dec 6, 2018

Added a test, verified that it fails without that fix.

@hroncok hroncok force-pushed the hroncok:nopem branch 2 times, most recently from 86b4cc9 to 4a5e1d2 Dec 6, 2018

Show resolved Hide resolved tests/test_virtualenv.py Outdated

@hroncok hroncok force-pushed the hroncok:nopem branch 3 times, most recently from a88216b to 3756c9b Dec 6, 2018

@hroncok

This comment has been minimized.

Contributor

hroncok commented Dec 6, 2018

OK, I have no idea what's the problem on Windows now or how to see any details 😢

Help me please?

@gaborbernat

This comment has been minimized.

Contributor

gaborbernat commented Dec 6, 2018

Paths on windows are using the \ separator instead of /. Use path.join to make your code OS agnostic (path.join(a, "src", "virtualenv_support")).

Show resolved Hide resolved tests/test_virtualenv.py Outdated

@hroncok hroncok force-pushed the hroncok:nopem branch from 3756c9b to 5aa16d5 Dec 6, 2018

@hroncok

This comment has been minimized.

Contributor

hroncok commented Dec 6, 2018

@gaborbernat

This comment has been minimized.

Contributor

gaborbernat commented Dec 6, 2018

Once we get the tests passing I would like to ask you to break up into a failing test only commit, and then another on top that does the source change and fixes it.

@hroncok hroncok force-pushed the hroncok:nopem branch 3 times, most recently from a2d6183 to bc4940c Dec 6, 2018

@hroncok

This comment has been minimized.

Contributor

hroncok commented Dec 6, 2018

Once we get the tests passing I would like to ask you to break up into a failing test only commit, and then another on top that does the source change and fixes it.

Shall I mark the test xfail in the first commit or not?


I have figured out that if the script I've changed fails, there is no error output anywhere in the test log, so I've temporarily enabled that output for now to see what's wrong on the Windows Python 3 code (maybe it's juts a different exception type). However the CI won't start.

@gaborbernat

This comment has been minimized.

Contributor

gaborbernat commented Dec 6, 2018

No need for xfail just let it fail. if you ammend your commit it my trigger a redirect.

@gaborbernat

This comment has been minimized.

@hroncok hroncok force-pushed the hroncok:nopem branch from 540bb77 to add1f2f Dec 6, 2018

Show resolved Hide resolved tests/test_virtualenv.py Outdated

@hroncok hroncok force-pushed the hroncok:nopem branch from add1f2f to 4fd87f4 Dec 6, 2018

hroncok added some commits Dec 5, 2018

Add test for missing pip/certifi's cacert.pem
See https://github.com/pypa/pip/blob/master/src/pip/_vendor/README.rst

It is acceptable to remove the pip/_vendor/requests/cacert.pem file from pip

Virtualenv should not error on that
Fix error with missing pip/certifi's cacert.pem
See https://github.com/pypa/pip/blob/master/src/pip/_vendor/README.rst

It is acceptable to remove the pip/_vendor/requests/cacert.pem file from pip

Virtualenv now doesn't error on that

@hroncok hroncok force-pushed the hroncok:nopem branch from 4fd87f4 to 5f9081e Dec 6, 2018

@gaborbernat gaborbernat merged commit a6436cc into pypa:master Dec 6, 2018

@gaborbernat

This comment has been minimized.

Contributor

gaborbernat commented Dec 6, 2018

thanks

@hroncok

This comment has been minimized.

Contributor

hroncok commented Dec 6, 2018

Thanks to you.

@hroncok hroncok deleted the hroncok:nopem branch Dec 6, 2018

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