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

Include gcc package in container #638

Closed
wants to merge 2 commits into from
Closed

Conversation

sc68cal
Copy link
Contributor

@sc68cal sc68cal commented Jan 3, 2024

Many python packages will compile native extensions and require GCC

@Alex-Izquierdo
Copy link
Contributor

If those packages are not required by this base image, we should avoid increasing the size of the image. Any other custom image that uses this one as a base can install any necessary components.

@sc68cal
Copy link
Contributor Author

sc68cal commented Jan 3, 2024

How do we handle the implicit coupling between those images and this base image? The issue is that this image uses a custom $USER_ID and authors of containers based off that have to switch between USER 0 to be able to do dnf install and then know that they have to switch back to $USER_ID so that their install doesn't get buggered up with path and permission issues.

Many python packages will compile native extensions and require GCC
@ttuffin
Copy link
Contributor

ttuffin commented Jan 8, 2024

@sc68cal what is the size of the image build after adding these packages?

@Alex-Izquierdo
Copy link
Contributor

How do we handle the implicit coupling between those images and this base image? The issue is that this image uses a custom $USER_ID and authors of containers based off that have to switch between USER 0 to be able to do dnf install and then know that they have to switch back to $USER_ID so that their install doesn't get buggered up with path and permission issues.

I don't think this a big deal, It is a common a good practice to use a non-root user as default and I think it is always expected to inspect the original source if you are going to alter/extend the original image. On the other hand, we should not add packages that are not strictly required only "just-in-case". Who extends the image is responsible to know what it is needed and how to do it.

@sc68cal
Copy link
Contributor Author

sc68cal commented Jan 8, 2024

@sc68cal what is the size of the image build after adding these packages?

With GCC and the Python development headers the image is 896MB on aarch64. The standard image, built from 5d4ebf1 is 710MB.

This is with podman via MacOS

@sc68cal
Copy link
Contributor Author

sc68cal commented Jan 8, 2024

@Alex-Izquierdo @ttuffin

I don't want to open an issue for this since it's a lot of mandatory fields for a feature request, but since we've broached the subject of custom images layered overtop of this image, can we consider setting APP_DIR to something different? Could we install ansible-rulebook and the related parts that make up ansible-rulebook either in /usr/local/ or /bin? Because if I'm looking to use this image as my base layer for my rulebooks I would have expected /app to be available for my use, instead of where ansible-rulebook is installed.

Or am I looking at this completely wrong and this container is meant exclusively for CI/CD for PRs against this repository?

I was originally maintaining my own Containerfile built on top of Debian but really don't want to be managing my own JRE install and all the other pieces that are needed to install ansible-rulebook - I was hoping for a layer that I could use that would have everything set up correctly and I could just worry about deploying and maintaining my code.

@Alex-Izquierdo
Copy link
Contributor

Hi @sc68cal The ansible-rulebook image is intended to be used as a base decision environment for eda-server as well as a standalone CLI tool (in addition to our CI workflows) so, while it is not its main purpose, the extensibility of the image is in the scope.

I think your request is reasonable. Look at the work of #639 done by @dhaustein
I think with the current stuff related with APP_DIR should be enough. Feel free to suggest there changes.

@Alex-Izquierdo
Copy link
Contributor

This change was finally added in #616

@sc68cal sc68cal deleted the include_gcc branch January 24, 2024 04:13
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.

3 participants