FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

python-setup: Fix venv creation in Ubuntu 22.04 by RasmusWL · Pull Request #1257 · github/codeql-action · GitHub

/ codeql-action Public

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

Merged
henrymercer merged 7 commits into
mainfrom
rasmuswl/fix-ubuntu22.04-venv-creation
Sep 21, 2022
ConversationCommits (7)ChecksFiles changed
Merged

python-setup: Fix venv creation in Ubuntu 22.04#1257
henrymercer merged 7 commits into
mainfrom
rasmuswl/fix-ubuntu22.04-venv-creation

Conversation

RasmusWL commented Sep 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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.

RasmusWL requested review from a team as code owners September 19, 2022 07:50
RasmusWL mentioned this pull request Sep 19, 2022
Closed
yoff previously approved these changes Sep 19, 2022
View reviewed changes

yoff left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

henrymercer reviewed Sep 20, 2022
View reviewed changes

henrymercer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Sep 21, 2022

Copy link
Copy Markdown
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 commented Sep 21, 2022

Copy link
Copy Markdown
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 commented Sep 21, 2022

Copy link
Copy Markdown
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 marked this pull request as draft September 21, 2022 09:40

RasmusWL commented Sep 21, 2022

Copy link
Copy Markdown
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 commented Sep 21, 2022

Copy link
Copy Markdown
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 commented Sep 21, 2022

Copy link
Copy Markdown
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 previously approved these changes Sep 21, 2022
View reviewed changes

henrymercer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me, just a copy editing suggestion

Comment thread python-setup/auto_install_packages.py Outdated
Apply suggestions from code review
32ca2cf 
Co-authored-by: Henry Mercer <henrymercer@github.com>

henrymercer commented Sep 21, 2022

Copy link
Copy Markdown
Contributor

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

henrymercer marked this pull request as ready for review September 21, 2022 14:07
henrymercer merged commit ff5ca12 into main Sep 21, 2022
henrymercer deleted the rasmuswl/fix-ubuntu22.04-venv-creation branch September 21, 2022 15:27
This was referenced Sep 21, 2022
Merged
Merged
RasmusWL added a commit that referenced this pull request Nov 29, 2022
python-setup: rely on new virtualenv for venv creation in Ubuntu 22.04
c62445d 
Removes the hotfix from #1257
RasmusWL mentioned this pull request Nov 29, 2022
Merged
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

henrymercer henrymercer approved these changes
yoff yoff left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

Back | FazBrowse Home | New Git URL