tianocore / containers

Repository to maintain and manage edk2 containers
Other
23 stars 27 forks source link

Introduce ubuntu20 build and dev images #13

Closed jgarver closed 2 years ago

jgarver commented 2 years ago

These are similar to the existing fedora images, but based on ubuntu20.

The build image is similar in spirit to the fedora build image. It can be used in automation.

A test image hasn't been introduced yet.

A dev image is included and is intended for direct use by an EDK2 developer. To use it, the developer must mount their home directory into the container. They may also mount the EDK2 workspace or simply place it in their home directory.

Issue #12

jgarver commented 2 years ago

I'm not even certain this is something you want to see in this repo. If not, we can host it in a different repo. No worries!

Also, I took some guesses about file names, structure, etc. to distinguish these images from the existing fedora images. I didn't want to change too much without feedback, but I'm happy to change more (or less). Just let me know how you'd like to see it.

cfernald commented 2 years ago

I'm not even certain this is something you want to see in this repo. If not, we can host it in a different repo. No worries!

Also, I took some guesses about file names, structure, etc. to distinguish these images from the existing fedora images. I didn't want to change too much without feedback, but I'm happy to change more (or less). Just let me know how you'd like to see it.

I think we should probably move to a subfolder style for supporting multiple image types. I'm also looking into some other docker images. @osteffenrh , thoughts?

osteffenrh commented 2 years ago

First: Thank you for contributing a dev image, @jgarver.

Yeah, we should add a folder structure and maybe (if possible) unify/simplify the GitHub actions workflow. Creating https://github.com/tianocore/containers/issues/14 for that.

osteffenrh commented 2 years ago

About the entry point shell script: I see what the goal is but find it somewhat unexpected. The user needs to know how to use it.

There is toolbox/toolbx (https://containertoolbx.org/) for example, that manages interactive container environments (mounting home folder, matching uid/gid, etc). It has only some small requirements on the image (https://containertoolbx.org/distros/). Could this be an alternative?

osteffenrh commented 2 years ago

Here is an example of a Debian toolbox image: https://github.com/containers/toolbox/tree/e27d7cafa45303100db91797179ecec1c4abb9a3/images/debian/unstable

osteffenrh commented 2 years ago

Folder structure and workflow in place now (#14)

jgarver commented 2 years ago

Thanks, @osteffenrh. I'll rebase when I get some cycles and also look at the rest of your comments.

jgarver commented 2 years ago

@osteffenrh @cfernald How do you feel about the "python" command in these containers? I've been avoiding it completely and prefer to either explicitly call python2 or python3, but I see you're calling "python" in test_build_edk2.py and I think you're assuming it's python3. I can setup the ubuntu container to match, but wanted to make sure that was the intent.

Python2 is finally EOL and I'm seeing more users and distros default "python" to python3, but I'm not sure what the EDK2 community would expect at this stage.

Also, looks like Debain/Ubuntu aren't shipping with /usr/bin/python any more, but it's easy for users to add one.

Here's the PEP, for reference: https://peps.python.org/pep-0394/

osteffenrh commented 2 years ago

Python2 is finally EOL and I'm seeing more users and distros default "python" to python3, but I'm not sure what the EDK2 community would expect at this stage.

The EDk2 python tools all use a reasonably new version of pyhton 3. 3.10 I think. This is also what I use as default.

jgarver commented 2 years ago

@osteffenrh I looked into toolbox. It would be a convenient replacement for the homegrown solution I've developed internally for our developers. However, it's not available in earlier versions of Ubuntu so installation would be cumbersome and a goal of the container-based dev env was to simplify developer onboarding.

That said, I think this comes down to how you folks would like to see your dev containers structured. If toolbox is your chosen approach, then we'll conform.

BTW, here's an SO discussion on the topic. My entrypoint looks like the second answer. https://stackoverflow.com/questions/57776452/is-it-possible-to-map-a-user-inside-the-docker-container-to-an-outside-user

osteffenrh commented 2 years ago

@jgarver, I do not have any strong opinion on that. Just want to make sure it is not too complicated or confusing. Not everybody has the same expectations and workflows/habits.

If it works I'd say lets put the image up and see.

jgarver commented 2 years ago

@osteffenrh Thanks for the response. I find this area of docker a bit lacking. "Run a container as the current user" would seem to be a common use case, but there doesn't seem to be proper support for it. After scouring the web, this entrypoint approach still seems the least magical without involving extra machinery. As an aside, too often, I found folks just opening their file permissions and building as root; I really don't want to do that.

I think it's ready to merge now. It's passing the checks and I've verified it still works as a foundation for our internal process. Hopefully, others find a use for it and we'll adjust the entrypoint based on feedback.

Finally, thanks for the workflow scripts, specifically test_build_edk2.sh. They were very helpful!

osteffenrh commented 2 years ago

I did not check the other .sh file.

jgarver commented 2 years ago

@osteffenrh Fixed. Thanks!

jgarver commented 2 years ago

Hi folks, are we ready to merge this? We have customers building their own containers and I'd like to spare them the trouble and use a common one. Thanks!

cfernald commented 2 years ago

Hi folks, are we ready to merge this? We have customers building their own containers and I'd like to spare them the trouble and use a common one. Thanks!

Hey @jgarver, sorry about the delays. My only outstanding reservation would be about the customized entrypoint logic. As Oliver mentioned different EDKII based projects have different development flows. For example, out projects don't use the EDK Repo tool. Would you be open to not adding the entrypoint into the base image and allowing the user to decide if they want to create it with the "--entrypoint" argument?

@osteffenrh , would this work for you as well?

jgarver commented 2 years ago

The scope of the entrypoint is just to match permissions, avoid running as root, and avoid having to chmod the build directory. But we can make this optional. I'd prefer to do it by offering two images. dev and devuser?

Not to point fingers, but consider these instructions for another edk2 docker container. The first step is to mkdir with perms "1777" (read/write by everyone). That's exactly what I want to avoid. https://github.com/xaionaro/edk2-builder-docker#quick-start

osteffenrh commented 2 years ago

Also from me: sorry for the late response.

I am OK with the permission dropping functionality of the entry point, since it can be bypassed. It should be documented though to prevent confusion.

The other point was the EDK2 repo tool, but it seems to be completely optional / it does not get in the way if not used explicitly. If so, I am OK with this PR.

jgarver commented 2 years ago

Thanks, @osteffenrh. How do you prefer to document the entry point functionality? And by "bypass" do you mean the "su" conditional? I could add an informational message at the end, just before "exec runuser". Something like: echo "Running as uid ${user_uid} (to run as root, prefix the command with 'su'").

Or I can reverse it and make runuser and the bulk of this entrypoint an opt-in. By default, it would behave similarly to the build container. But with an option or command prefix, require the homedir and drop perms.

jgarver commented 2 years ago

Oh, and yes, edkrepo is just a tool, much like the toolchains, virtualenv, and git. Some builds, like ours, will depend on it. Others will not. So, I guess it's a balance of keeping the image small vs. increasing its range of usability.

cfernald commented 2 years ago

Assuming the intention would be to not run the entrypoint at all, the user could just use --entrypoint='' to their container creation.

Either way, it might be good to document this, but I'll sign off on this for now.

osteffenrh commented 2 years ago

I was thinking about adding a readme per image.