-
Notifications
You must be signed in to change notification settings - Fork 170
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
EL7 compatibility fixes #344
Conversation
@miabbott mind doing a rebase from master, and let's put this through an Upshift build? I think we really close to doing EL7 builds there. |
@@ -42,7 +42,7 @@ def oscontainer_extract(containers_storage, src, dest, | |||
if commit is None: | |||
raise SystemExit("Failed to find label '{}'".format(OSCONTAINER_COMMIT_LABEL)) | |||
iid = inspect['Id'] | |||
print(f"Preparing to extract cid: {iid}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm - we are running with python3 in el7 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my commit message...
TL;DR - we mangle this script to use Python2 on EL7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I found this:
Line 100 in 460741e
sed -i 's|^#!/usr/bin/env python3|#!/usr/bin/python2|' src/cmd-oscontainer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, using .format()
is one of the original guidelines noted in #271 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, using
.format()
is one of the original guidelines noted in #271 wink
yep. that also says * target 3.4
😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. that also says
* target 3.4
It's not a bad target 😉
The version of `qemu-kvm-rhev` on EL7 is more recent than `qemu-kvm`, so let's use that.
In the EL7 version of the `cosa` image, the `cmd-oscontainer` script gets mangled to use Python2, so f-strings are not an option here.
Must have just missed your changes getting merged...pushed a rebase ⬆️ |
LGTM. Merging. |
Against my better judgement, I've been trying to get the EL7 build of the
cosa
image to be usable. This may not be all the required changes, but it got me to a point where I could build the image and then runfetch/build/run/shell/compress
successfully.The last papercut noted here was solved by @cgwalters suggestion to use
qemu-kvm-rhev
. (On that note, #329 is required for these fixes to be useful)I also had to tweak the
deps.txt
to getsyslinux
successfully installed (although, I did not test generating an ISO)Finally, the
cmd-oscontainer
script needed a small change to use.format()
instead of f-strings.Depends: #329