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

python-setup: Fix venv creation in Ubuntu 22.04 #1257

Merged
merged 7 commits into from Sep 21, 2022

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Sep 19, 2022

Fixes #1249

As described here, when using Ubuntu 22.04 with new enough versions of setuptools (60.0.0+), the virtual environment created with virtualenv will put binaries in <venv-path>/local/bin instead of <venv-path>/bin.

Next release after 20.16.5 of virtualenv will include a fix for this (PR).

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

yoff
yoff previously approved these changes Sep 19, 2022
Copy link

@yoff yoff left a comment

Choose a reason for hiding this comment

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

From how I read the given context (thanks!) this looks fine to me.

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Seems reasonable. I'd like to start running these tests on Ubuntu 22.04, so we can verify this is fixed and to catch it if it breaks again in the future. Could you add ubuntu-22.04 to the matrix in python-deps.yml (and any other relevant tests)?

@RasmusWL
Copy link
Member Author

Could you add ubuntu-22.04 to the matrix in python-deps.yml (and any other relevant tests)?

Done 👍 (but only for the python-setup tests)

@henrymercer
Copy link
Contributor

I don't think Python 2 is preinstalled in the ubuntu-22.04 virtual environment. Would it be reasonable to add the combination of python_version: 2 and os: ubuntu-22.04 to the exclude listing in the matrix, or should we install Python 2 using actions/setup-python?

@RasmusWL
Copy link
Member Author

I don't think Python 2 is preinstalled in the ubuntu-22.04 virtual environment. Would it be reasonable to add the combination of python_version: 2 and os: ubuntu-22.04 to the exclude listing in the matrix, or should we install Python 2 using actions/setup-python?

we have a bit of an internal discussion on how to handle this. Let me get back to this PR once we have figured out what to do.

@RasmusWL RasmusWL marked this pull request as draft September 21, 2022 09:40
@RasmusWL
Copy link
Member Author

this should fail the Ubuntu 22.04 test, but just want to be able to point to a log file to show that the logic works before disabling the test. If only I had designed the test system to be more flexible, maybe we could have kept the test alive under a form of expected failure mode 🤔

@henrymercer
Copy link
Contributor

Another way to test this might be adding a step that calls actions/setup-python with python-version: 2, that is only executed when os == ubuntu-22.04 and python_version == 2?

@RasmusWL
Copy link
Member Author

Example of failure here: https://github.com/github/codeql-action/actions/runs/3097944471/jobs/5015231587#step:4:140 (revealed that I had forgotten a few spaces)

henrymercer
henrymercer previously approved these changes Sep 21, 2022
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a copy editing suggestion

python-setup/auto_install_packages.py Outdated Show resolved Hide resolved
Co-authored-by: Henry Mercer <henrymercer@github.com>
@henrymercer
Copy link
Contributor

Taking this out of draft since that reruns the PR checks.

@henrymercer henrymercer marked this pull request as ready for review September 21, 2022 14:07
@henrymercer henrymercer merged commit ff5ca12 into main Sep 21, 2022
@henrymercer henrymercer deleted the rasmuswl/fix-ubuntu22.04-venv-creation branch September 21, 2022 15:27
RasmusWL added a commit that referenced this pull request Nov 29, 2022
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.

Exception in configuring venv when the project has a setup.py file
3 participants