libvirt: Support Ubuntu 23.04+ #2005
Conversation
|
I've manually tested this on 22.04 and 24.04 and it all (non-kbs) test passed for me |
|
It looks like in the CI the shell reload to pick up the new path isn't working, which might be to do with GHA isolating PATH between steps, so we might need some PATH update in the workflow, so putting this in draft for now. |
I hope my workflow commit will do the trick. I'm not sure we can test it though, so maybe the best plan is to split out the workflow change and deliver it separately? |
stevenhorsman
force-pushed
the
ubuntu-24.04-kcli-install
branch
from
a7ff93e to
4de1548
Compare
stevenhorsman
force-pushed
the
ubuntu-24.04-kcli-install
branch
from
4de1548 to
9f74299
Compare
|
Now we can run the libvirt x86 tests on a github hosted runner. I'm able to test this in my fork and it passes: https://github.com/stevenhorsman/cloud-api-adaptor/actions/runs/12047365356/job/33590586948, so this is ready to review and merge directly. |
stevenhorsman
force-pushed
the
ubuntu-24.04-kcli-install
branch
from
9f74299 to
6293e3f
Compare
stevenhorsman
force-pushed
the
ubuntu-24.04-kcli-install
branch
from
6293e3f to
f59d330
Compare
|
I rebased this and tested it on a ubuntu 24.04 VM: |
There was a problem hiding this comment.
Hi @stevenhorsman ! Sorry for taking longer to review it. LGTM. Thanks!
|
|
||
| if [ ${OS_DISTRO} == "ubuntu" ]; then | ||
| # Reload shell so that pipx install PATH is available | ||
| exec $SHELL |
There was a problem hiding this comment.
i'm curious. why don't we need this at line 87? and if we do, why don't we source the env directly after installation of pipx?
There was a problem hiding this comment.
Good question - I wrote this code in August, so can't complete remember the reason. I think it was because pipx ensurepath meant the path was only updated in the script's shell, which works for inside this script, but meant that the follow steps didn't have an updated path with the exec bash. Let me play with this again and try and refresh my memory to confirm.
I can just put this exec into line 87 instead of the ensurepath and see is that works as well?
There was a problem hiding this comment.
maybe I'm missing something, but the code looks like a noop. it re-sources .profile or something and then it's just discarded because the script (and the sh process, I assume).
There was a problem hiding this comment.
So I think I'm remembering the issues I saw that lead me to this:
When we run pipx ensurepath the output is:
/root/.local/bin has been been added to PATH, but you need to open a new terminal or re-login for this PATH change
to take effect.
You will need to open a new terminal or re-login for the PATH changes to take effect.
Otherwise pipx is ready to go! ✨ 🌟 ✨
If we check our PATH then:
# echo $PATH
/usr/local/go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
It isn't added, but if we reload the shell and re-do it then it works:
# exec $SHELL
# echo $PATH
/usr/local/go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/root/.local/bin
However re-loading the shell within the script means that all further output doesn't show up, so that's why I added the reload to the end of the script to avoid that loss of output.
I could remove it entirely and we'd then need to ask the user to reload their shell, so I chose to do it for them to remove a step.
There was a problem hiding this comment.
I don't fully understand this, so I've tried to give a cut down version of the script to demo:
# cat test.sh
pipx ensurepath
echo "PATH before reload: $PATH"
exec $SHELL
echo "We don't see this"
# ./test.sh
/root/.local/bin has been been added to PATH, but you need to open a new terminal or re-login for this PATH change
to take effect.
You will need to open a new terminal or re-login for the PATH changes to take effect.
Otherwise pipx is ready to go! ✨ 🌟 ✨
PATH before reload: /usr/local/go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
# echo $PATH
/usr/local/go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/root/.local/bin
There was a problem hiding this comment.
Sorry, I still don't follow - when I leave the scope of the script why would it (without running exec $SHELL in that script) have a reload as I'm just returning to the shell scope I was already in before calling it?
There was a problem hiding this comment.
my point would be:
you can run exec $SHELL in the script, but that will only be valid in the child process that evaluates the script, not the script-invoking shell.
There was a problem hiding this comment.
So my point is that (as I've explained and showed example commands above), I would expect that behaviour, but it's not the case, hence the exec $SHELL is not a no-op
There was a problem hiding this comment.
we will probably not get to the bottom off this mystery in this PR 😅 but what I suspect happens in your example above:
in the script you open a new shell (and by that you replace your current process with it). this new shell has the PATH set. if you ctrl-d out of that shell, however, the PATH will be lost again because you're back at the shell that called the script initially. Now, that' s admittedly not a full noop, at least not in a TTY, because it opens a new shell, but if we emulate a github action step calling your script by shortfusing stdin the new shell will immediately exit:
$ bash test.sh < /dev/null
~/.local/bin has been been added to PATH, but you need to open a new terminal or re-login for this
PATH change to take effect.
You will need to open a new terminal or re-login for the PATH changes to take effect.
Otherwise pipx is ready to go! ✨ 🌟 ✨
$ echo $PATH | grep "local/bin$"
$ # ☁️
$ bash # new shell
magnuskulke@DESKTOP-2KAPMGM:~$ echo $PATH | grep -oh "local/bin$"
local/bin
$ # 🌞 There was a problem hiding this comment.
Yeah this behaviour is for the local install, not the GHA, which is more straightforward as there is clear shell separation!
In newer versions of Ubuntu running `pip3 install kcli` results in
the following error:
```
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.
...
```
Solutions seem to be either use a virtual environment,
install the package with apt (which isn't available for us as we need),
or use pipx if installing an application, which we are.
This attempts to use the logic to install with pipx, including a
shell reload at the end of the script to force the PATH to be updated
Signed-off-by: stevenhorsman <[email protected]>
With pipx to install kcli, we now need `${HOME}/.local/bin`
to be added to `PATH`, so that we can pick up the install,
so try exporting this to $GITHUB_PATH
Signed-off-by: stevenhorsman <[email protected]>
stevenhorsman
force-pushed
the
ubuntu-24.04-kcli-install
branch
from
f59d330 to
35c2fd0
Compare
There was a problem hiding this comment.
LGTM
I guess other distro like centos/rhel can also use pipx, maybe we can change it later. Thanks! @stevenhorsman
Yeah, I was just trying to unblock David's development with this fix, so targeted ubuntu as the distro he had issues with, but it might also be useful for newer versions of fedora and similar. Maybe someone like @wainersm who I think uses it might be able to comment? |
In newer versions of Ubuntu running
pip3 install kcliresults in the following error:Solutions seem to be either use a virtual environment, install the package with apt (which isn't available for us as we need), or use pipx if installing an application, which we are.
This attempts to use the logic to install with pipx, including a shell reload at the end of the script to force the PATH to be updated