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

Adding Python venv #78

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Adding Python venv #78

merged 1 commit into from
Oct 10, 2024

Conversation

erikbosch
Copy link
Contributor

Needed after implicit update to ubuntu 24.04

Needed after update to ubuntu 24.04

Signed-off-by: Erik Jaegervall <[email protected]>
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.44%. Comparing base (f756202) to head (542a400).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #78   +/-   ##
=======================================
  Coverage   50.44%   50.44%           
=======================================
  Files          31       31           
  Lines       11702    11702           
=======================================
  Hits         5903     5903           
  Misses       5799     5799           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@erikbosch erikbosch marked this pull request as ready for review October 10, 2024 10:53
Comment on lines +99 to +102
- uses: actions/setup-python@v5
# Needed for pip
with:
python-version: '3.12'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really using a pip virtual environment? It looks like the problem is fixed, by installing a separate python installation which prevents the system python installation from being polluted - which might be a workable solution.

That the fix works is obvious. Just want to have a common understanding here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess you are right - as I understand it installs Python and adds it to path. It does not in itself initiate/activate a virtual environment. Setup-python has however some support for caching so in one way I think it may reuse certain parts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some other repos like https://github.com/eclipse-kuksa/kuksa-python-sdk/blob/main/.github/workflows/dash.yaml we actually use a virtual environment. From practical perspectives I generally do no think that the difference will be that big as we typically do not need multiple different Python environments in the same workflow.

Copy link
Contributor

@wba2hi wba2hi Oct 10, 2024

Choose a reason for hiding this comment

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

Guess you are right - as I understand it installs Python and adds it to path. It does not in itself initiate/activate a virtual environment. Setup-python has however some support for caching so in one way I think it may reuse certain parts

Agree. But caching seems to be disabled in our case, because from what I understand it needs to be explicitely enabled by using:

cache: 'pip' // pipenv // 

see: https://github.com/actions/setup-python?tab=readme-ov-file#caching-packages-dependencies

Again, not telling here that we have to use a cache or not use cache (both will solve problems and create others), just pointing out things :)

In some other repos like https://github.com/eclipse-kuksa/kuksa-python-sdk/blob/main/.github/workflows/dash.yaml we actually use a virtual environment. From practical perspectives I generally do no think that the difference will be that big as we typically do not need multiple different Python environments in the same workflow.

I implemented that stuff based on some commands which were provided to me, so take it with a grain of salt :D It's probably still not the best way to handle that stuff. However in that case it was mandatory to have a clean / unpolluted python instance because otherwise we would point out libraries which are required by the system as libraries used by our software.

Personally I'm fine with the approach used here. As said, I just wanted to have a common understanding. The Commit Title somehow suggests that we use a venv and maybe even what we have now is called a venv - I'm no Python expert here. I was just directly thinking about the pip venv thingy and was somehow missing in my head and in this commit where it's activated :)

@SebastianSchildt
Copy link
Contributor

What was the issue with not doing this? Incompatible Python version, or just no pip oob? Generally of course "pinning" some Python version is likely a good idea even if it weren't broken before

@wba2hi
Copy link
Contributor

wba2hi commented Oct 10, 2024

What was the issue with not doing this? Incompatible Python version, or just no pip oob? Generally of course "pinning" some Python version is likely a good idea even if it weren't broken before

We are using ubuntu-latest and it looks like the underlying ubuntu version was upgraded, which lead to a new python version being applied which warns / fails with an error, when the system python is used for installation of requirements:

error: externally-managed-environment

× This environment is externally managed
╰─> To install Python packages system-wide, try apt install
    python3-xyz, where xyz is the package you are trying to
    install.
    
    If you wish to install a non-Debian-packaged Python package,
    create a virtual environment using python3 -m venv path/to/venv.
    Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make
    sure you have python3-full installed.
    
    If you wish to install a non-Debian packaged Python application,
    it may be easiest to use pipx install xyz, which will manage a
    virtual environment for you. Make sure you have pipx installed.
    
    See /usr/share/doc/python3.12/README.venv for more information.

note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.
hint: See PEP 668 for the detailed specification.
Error: Process completed with exit code 1.

Copy link
Contributor

@wba2hi wba2hi left a comment

Choose a reason for hiding this comment

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

LGTM

@erikbosch
Copy link
Contributor Author

What was the issue with not doing this? Incompatible Python version, or just no pip oob? Generally of course "pinning" some Python version is likely a good idea even if it weren't broken before

We use ubuntu-latest in most workflows, and github recently changed so that ubuntu-latest points to ubunto-24.04 instead of ubuntu-22.04

And then when doing pip install ... we now get errors like below.

It seems that pip in ubuntu-24.04 is more strict than in 22.04

error: externally-managed-environment

× This environment is externally managed
╰─> To install Python packages system-wide, try apt install
    python3-xyz, where xyz is the package you are trying to
    install.
    
    If you wish to install a non-Debian-packaged Python package,
    create a virtual environment using python3 -m venv path/to/venv.
    Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make
    sure you have python3-full installed.
    
    If you wish to install a non-Debian packaged Python application,
    it may be easiest to use pipx install xyz, which will manage a
    virtual environment for you. Make sure you have pipx installed.
    
    See /usr/share/doc/python3.12/README.venv for more information.

note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.
hint: See PEP 668 for the detailed specification.

See https://github.com/eclipse-kuksa/kuksa-databroker/actions/runs/11270488560

@erikbosch erikbosch merged commit ecfeb0b into eclipse-kuksa:main Oct 10, 2024
23 checks passed
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.

3 participants