Use the same version of Python everywhere, and let it be specified in a single place#1022
Conversation
Including just the version number makes the file understood by more tools and environments, for example, by `actions/setup-python`, which allows us to specify the version of python in a single place.
sigmundpetersen
moved this from All the things 💤
to Code review 🔎
in OFN Delivery board
| if pyenv virtualenvs 2>&1 | grep -q "not installed"; then | ||
| pyenv virtualenv "$python_version" "$env_version" | ||
| pyenv virtualenv "$python_version" "ofn-install-$python_version" | ||
| fi |
There was a problem hiding this comment.
Hm, this is now broken. The virtualenv plugin was creating a "new python version" based on another version. It used the .python-version file to track which python version is currently used. So since that's just the original version now, the if statement is always false and we never create a new virtualenv. But even if we created a new environment, we would need to use it.
The suffix in the version was indicating the Ansible version. That allowed us to switch between Ansible versions by choosing a different virtualenv. Now I can still set pyenv shell ofn-install-3.10.16 to use that version without overriding the .python-version file. But the pyenv-install script isn't really supporting me in this.
Maybe we could write another script that looks at the Ansible version in requirements.txt and set the right virtualenv. You could use it like this:
source bin/virtualise-pyenv
cat bin/virtualise-pyenv
python_version="$(cat .python-version)"
ansible_version="$(grep "^ansible==" requirements.txt)"
venv="$python_version-$ansible_version"
if !pyenv shell "$venv"; then
pyenv virtualenv "$venv"
pyenv shell "$venv"
fi
That's only a temporary state but that's okay, I guess. I don't know. Maybe something to try when we upgrade Ansible next.
I think, for now, you can just delete that section because it's not doing anything. And then the virtualenv thing is optional for devs that want to use it.
There was a problem hiding this comment.
Hm, this is now broken. The virtualenv plugin was creating a "new python version" based on another version. It used the .python-version file to track which python version is currently used. So since that's just the original version now, the if statement is always false and we never create a new virtualenv. But even if we created a new environment, we would need to use it.
Oh, sorry, I should've actually installed pyenv and try this myself. So if I understand correctly, python virtualenvs actually uses .python-version under the hood, is that it?
The suffix in the version was indicating the Ansible version. That allowed us to switch between Ansible versions by choosing a different virtualenv. Now I can still set pyenv shell ofn-install-3.10.16 to use that version without overriding the .python-version file. But the pyenv-install script isn't really supporting me in this.
I think I misunderstood this, too 😅. I though 3.10.16-a8 meant some 3.10.16 prerelease of python, but -a8 actually has to do with the version of Ansible, correct?
I think, for now, you can just delete that section because it's not doing anything. And then the virtualenv thing is optional for devs that want to use it.
I'd say, given I have no idea what I'm doing, I should close this PR. I definitely do not want to break your development environment!
There was a problem hiding this comment.
Yes, correct, but your PR still has some value. The virtualenv setup was added by one developer to switch environments more easily but it's not essential.
Given that virtualenv is messing with the .python-version file and we need it only for development, I would go with your PR and manage virtualenv manually in dev.
Hm, it does mean that we can't upgrade Ansible without updating the Python version as well or wipe Ansible and re-install it. Anyway, I don't want to update the python version and many places either, so let's go ahead with your PR, I think.
There was a problem hiding this comment.
I am not really across virtualenv to be honest, but I am happy to leave that to devs to figure out when they need it.
Do we need to update the README if we go ahead ?
There was a problem hiding this comment.
This seems like a good change to me, I like the idea of using a standard version number in .python-version.
From what I understand, we're still specifying the ansible version in requirements.txt, so a new installation should select the right version.
But for future updates, we would need to manually ensure we have the right version?
Maybe a better way to do that is with a different tool like asdf, a version management tool (which I only learnt about today), that can read the .python-version file and seems to support ansible versions too.
There was a problem hiding this comment.
Oh, i see that this might still need some work. Of course, it should be tested in a dev environment. I'm happy to give that a go once it's ready.
There was a problem hiding this comment.
So, how should I be updating this PR? Remove pyenv virtualenv usage completely? Indeed having a .python-version that can be understood by other tools like mise or asdf is part of the motivation for my PR.
There was a problem hiding this comment.
Hm I'm not sure, but I think what we want is dependency management for Python, just like we have Bundler for Ruby. Ansible is a dependency and we would like to lock the version in the same way.
I guess we were using a script with virtualenv to achieve that, but maybe there is a better more standard tool that allows us to keep .python-version clean. A quick search suggests pipenv, or maybe uv.
Or, now we're on a newish version of Ansible, maybe we could just try to keep up to date and not worry about locking it?
There was a problem hiding this comment.
I see! I guess the best way to do that would be to migrate to a tool like uv, indeed.
I'm unsure about the current role of -a8 in .python-version. If you want to change the version of ansible, you still have to change requirements.txt file, right? So, again, multiple sources of truth potentially getting out sync. It seems better to change the requirements.txt file and let the globally installed version of ansible be replaced by pip install -r requirements.txt. If you want a specific virtualenv to use some specific version of ansible without overwriting the global version, you can still create it manually.
So overall, I'd say it's best to remove explicit pyenv virtualenv usages (I think that's what @mkllnk suggested) and let virtualenvs be handled manually (or by dev's tool of choice, like uv). That way the project becomes more standard by having a standard .python-version file.
But as I said, I don't want to break your dev envs 😅.
github-project-automation
Bot
moved this from Code review 🔎
to In Progress ⚙
in OFN Delivery board
| if pyenv virtualenvs 2>&1 | grep -q "not installed"; then | ||
| pyenv virtualenv "$python_version" "$env_version" | ||
| pyenv virtualenv "$python_version" "ofn-install-$python_version" | ||
| fi |
There was a problem hiding this comment.
This seems like a good change to me, I like the idea of using a standard version number in .python-version.
From what I understand, we're still specifying the ansible version in requirements.txt, so a new installation should select the right version.
But for future updates, we would need to manually ensure we have the right version?
Maybe a better way to do that is with a different tool like asdf, a version management tool (which I only learnt about today), that can read the .python-version file and seems to support ansible versions too.
mkllnk
left a comment
mkllnk
left a comment
There was a problem hiding this comment.
I just added a commit to accept your change of not using virtualenv by default but have a new script to use it optionally. That means that our development is as flexible as before and we stay compatible with all the standard tools. We are now also open to other managers.
|
The failed test is due to an issue in our ofn code base. The python setup seems all good here. |
github-project-automation
Bot
moved this from In Progress ⚙
to Done
in OFN Delivery board
|
Thanks @mkllnk! |
5 participants
By keeping a more standard
.python-versionfile,actions/setup-pythonwill respect it and does not need a specific input with the version of Python. That should ensure the same version is used locally and in CI.