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

Use module name (-m) to run pytest and pip #148

Closed
wants to merge 1 commit into from

Conversation

dosas
Copy link

@dosas dosas commented Jan 10, 2024

when using python version other than 3.8
pip and pytest were not using the correct python version

@mdellweg
Copy link
Member

And how does python3 know the right one to choose?
I believe with multiple versions of python installed in one container there is something going wrong here.

@dosas
Copy link
Author

dosas commented Jan 10, 2024

The switch python script takes care of that, it updates the alternative.

It is no problem to have multiple python versions as long as python -m is used.

@mdellweg
Copy link
Member

It is no problem to have multiple python versions as long as python -m is used.

I can see how it works, but the result looks more like a space station to me than a container.

@dosas
Copy link
Author

dosas commented Jan 10, 2024

This PR is not about changing the architecture or workflow of oci-env. It tries to provide a fix for an 'advertised' functionality, the possibility to use a different python version than 3.8. This functionality was broken an unnoticed.

If you have general issues with this workflow or architecture this is out of scope for this PR.
You are welcome to provide an alternative solution.

when using python version other than 3.8
pip and pytest were not using the correct python version
@dosas dosas force-pushed the fix/python-module-name branch from c5f6b95 to affa038 Compare January 10, 2024 15:57
@mdellweg
Copy link
Member

I guess switch python is broken then. This line is supposed to do it.

update-alternatives --install /usr/bin/pip3 pip3 /usr/bin/pip-${NEWV} 1

@dosas
Copy link
Author

dosas commented Jan 11, 2024

No, I disagree. The only way to have the correct python version for pip and pytest is using -m flag.

@mdellweg
Copy link
Member

I see it adding the alternative for pip3. Looks like it's missing to put one for pip too.

@dosas
Copy link
Author

dosas commented Jan 11, 2024

So we have two possibilities:

  1. Run pip and pytest and hope they chose the correct python version
    or
  2. Run the correct python version and let it invoke pip and pytest

I do not see any point in favor the first one.

If it works we can additionally add pip to switch_python or change every occurrence of pip to pip3.

@mdellweg
Copy link
Member

It is ridiculous in the first place to have multiple versions of python in one container fighting over precedence.
If you can find a way to fix up switch python, that might be an acceptible stop gap, but in the long run, when you want another python, start with a different base image.
When you do this, think about a user calling oci_env shell. They don't understand that pip or pytest is the wrong command to use.

@dosas
Copy link
Author

dosas commented Jan 15, 2024

If it is 'ridiculous' to have multiple versions of python, the logical option would be to remove the broken switch_python script.

@dkliban
Copy link
Member

dkliban commented Jan 15, 2024

@jctanner are you using the feature that lets you switch python versions? It doesn't seem to be working correctly. We are considering removing it all together.

@jctanner
Copy link
Contributor

We use it all the time with no issue https://github.com/ansible/galaxy_ng/blob/master/profiles/base/Dockerfile

@dosas
Copy link
Author

dosas commented Jan 15, 2024

We use it all the time with no issue https://github.com/ansible/galaxy_ng/blob/master/profiles/base/Dockerfile

That is because of the -m option :D

Are you invoking tests (unit/functional) with it?

@jctanner
Copy link
Contributor

jctanner commented Jan 15, 2024

@dosas no we aren't using it for functional or unit testing. The functional and unit tests run in CI based on the ansible playbooks created by the pulp plugin template. It builds an image and runs a container via ansible to do all that work, including a matrix with s3/azure/fs backing filestores. The galaxy team does not tend to write functional tests in the galaxy_ng repo. Instead, in our CI we also have integration tests (not pulp native testing) that do use the oci profiles+image.

@cutwater
Copy link

@dosas @mdellweg I wonder why can't oci-env just use venv created by required version of python. This will effectively fix all lookup issues and will not require tampering with system level python?

@mdellweg
Copy link
Member

We provide different base images with different python versions for this. Closing now.

@dosas @mdellweg I wonder why can't oci-env just use venv created by required version of python. This will effectively fix all lookup issues and will not require tampering with system level python?

This would still need multiple python versions to coexist in the same container.

@mdellweg mdellweg closed this Feb 15, 2024
@dosas
Copy link
Author

dosas commented Feb 17, 2024

For reference pulp/pulp-oci-images#586

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.

5 participants