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

CONTRIBUTING: Ensure pip gets upgraded; install wheel #3060

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EricCousineau-TRI
Copy link
Collaborator

Homogenize how to activate venv (use source instead of .)

Description

While briefly investigating a possible new issue (#2972 (comment)), was doing my local build env. Re-read CONTRIBUTING, and noted the followigng:

  • When following current version (484b0f0), noted that nominal pip version pip-9.0.1, as installed by Ubuntu 18.04's python3-venv package, was emitting warnings like Cache entry deserialization failed, entry ignored (due to version mismatch).
  • I know in other workflows, it can be useful to have wheel.
  • Ideally, we should choose one way to activate.

This addresess those points.

Suggested changelog entry:

N/A

Homogenize how to activate venv (use `source` instead of `.`)
@henryiii
Copy link
Collaborator

Why install wheel? Users should use pyproject.toml, wheel is an implementation detail that shouldn't be installed by hand unless you are dealing with legacy packages that don't have pyproject.toml's. If one of our dependencies needs it, perhaps? Hmm, if it is installing a wheel, but an old wheel, and the minimums are not specified in pyproject.toml, it will reuse the cached wheel - is this installing wheel, though? I would assume not.

The embedded pip that's installed by the ensurepip (and therefore venv) module can be out of date (especially on package managers that hack around with the embedded pip, like Debian), so updating that is a good idea. But why not use --upgrade-deps?

Comment on lines 60 to +62
python3 -m venv venv
source venv/bin/activate
pip install -U pip wheel
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
python3 -m venv venv
source venv/bin/activate
pip install -U pip wheel
python3 -m venv --upgrade-deps venv
source venv/bin/activate

Not sure when this was added, but guessing it's been around for a while? Using virutalenv ensures a recent versions of everything and is faster, but is not builtin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ouch, this was added in Python 3.9. One of the problems of being on a Mac... ;)

$ docker run --rm -it quay.io/pypa/manylinux_2_24_x86_64:latest bash
# python3.6 -m venv --help
# python3.7 -m venv --help
# python3.8 -m venv --help
# python3.9 -m venv --help

@@ -59,6 +59,7 @@ system with CMake 3.14+:
```bash
Copy link
Collaborator

@henryiii henryiii Jun 25, 2021

Choose a reason for hiding this comment

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

Suggested change
```bash
```console

Not something for now, but these should likely be changed (and prompts added).

Copy link
Collaborator

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

Okay, fine with me, we can't support --upgrade-deps unless we make this 3.9+ only, and installing wheel is likely not needed, but if you really want it, I'll leave that up to you.

We might want to add a noxfile.py, that might make "fly-by" contributing easier.

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.

2 participants