-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix: require Python >= 3.7, add requirements.txt #5
Conversation
Certain features like process.Popen()'s text= kwarg require Python >= 3.7.
Our current Dockerfiles for dnf-based builds use python 3.11 and we have instructions for using the prebuilt container rather than messing with python versions on the head node (examples/README.md). Should we also amend our instructions to make the container usage the preferred usage while also adding a guard for python versions that are too old in the code itself? |
Container usage can be the preferred way to run image-build and we should be clear in the instructions, but usage should not be limited to the container. For running bare-metal, I think we should have guards (and also instructions) since the code will definitely not run on any Python earlier than 3.7. |
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.
LGTM
It would be good to see an update to the README that encourages use of the container as part of this PR or as a new PR/Issue to follow quickly
Using `buildah unshare bash -c` requires the user to enclose the image-build command in quotes when running the container, e.g: docker run ghcr.io/openchami/image-build 'image-build --config ...' Removing `bash -c` removes this requirement so that the above command can be ran as: docker run ghcr.io/openchami/image-build image-build --config ... If running multiple commands is needed, you can: docker run ghcr.io/openchami/image-build bash -c 'echo test; image-build --config ...'
While testing running the container, I found some peculiarities. For instance, the entire I also noticed that, when running with Docker, I would get:
Adding I've pushed commits with these changes and would like to get them reviewed. |
We've decided to use Podman as the officially supported way to run the container due to the nature of nested containers. |
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.
LGTM
Certain features like process.Popen()'s text= kwarg require Python >= 3.7. Add a runtime check for it.
Also, add a
requirements.txt
and instructions for using it to install all needed Python dependencies.