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

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

Merged
merged 2 commits into from Dec 6, 2018

Conversation

hroncok
Copy link
Contributor

@hroncok 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

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.

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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

hroncok commented Dec 6, 2018

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

@hroncok hroncok force-pushed the nopem branch 2 times, most recently from 86b4cc9 to 4a5e1d2 Compare December 6, 2018 14:33
tests/test_virtualenv.py Outdated Show resolved Hide resolved
@hroncok hroncok force-pushed the nopem branch 3 times, most recently from a88216b to 3756c9b Compare December 6, 2018 15:17
@hroncok
Copy link
Contributor Author

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
Copy link
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")).

@hroncok
Copy link
Contributor Author

hroncok commented Dec 6, 2018

@gaborbernat
Copy link
Contributor

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 nopem branch 3 times, most recently from a2d6183 to bc4940c Compare December 6, 2018 17:31
@hroncok
Copy link
Contributor Author

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
Copy link
Contributor

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

@gaborbernat
Copy link
Contributor

tests/test_virtualenv.py Outdated Show resolved Hide resolved
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
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
@gaborbernat gaborbernat merged commit a6436cc into pypa:master Dec 6, 2018
@gaborbernat
Copy link
Contributor

thanks

@hroncok
Copy link
Contributor Author

hroncok commented Dec 6, 2018

Thanks to you.

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.

cool

@hroncok hroncok deleted the nopem branch December 6, 2018 22:52
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.

None yet

2 participants