-
Notifications
You must be signed in to change notification settings - Fork 168
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
tools: Fix tag for eve-mkimage-iso-efi #4662
Conversation
Commit ee15f4d introduced changes to fetch the eve-mkimage-iso-efi container for the right target architecture, the fix was needed to generate ISO images for arm64. However, the ARCH prefix was missing for the linuxkit pkg tag resolution. This commit adds the ARCH prefix just as it's done in the tools/parse-pkgs.sh script. Signed-off-by: Renê de Souza Pinto <[email protected]>
Why do we need the |
AFAIK we use that with packages that must be loaded to docker, for instance,
here I just did the same thing done in |
Yeah, that probably is wrong, too, but it is not your problem; it long predates this. If But this shouldn't hold you up. I will look at what is breaking here. |
I took another look at how this is used, I don't understand what doesn't work.
That line has an explicit What is not working that this is needed? |
Because |
Why would it do that? Do you mind linking where it does? |
I think this is linuxkit behavior, isn't? I don't see if this is explicitly set... FWIW, just ran manually the following: build-tools/bin/linuxkit pkg build --docker --platforms linux/amd64 --build-yml build.yml pkg/mkimage-iso-efi/ Output:
Exported container is
Edit:
However, if I build for arm64: build-tools/bin/linuxkit pkg build --docker --platforms linux/arm64 --build-yml build.yml pkg/mkimage-iso-efi/ Output:
We do have the following image exported:
But no TAG to |
Yeah, I see that. That is because of how docker stores (or stored, this is a bit changed) images. It doesn't actually store the index and the images for all architectures; when you do If you want to have the images for different architectures locally, you have to give them different tags. Hence, |
Having understood that, let's say I am running Why is that not enough that I need this PR? If I want to run |
Thanks, I think it's clear now. To your question: I do run these containers for arm64 a lot of my x86 host to develop and test arm64 stuff. For instance, for the mkimage-iso-efi I was using to generate ISO for arm64 (and test my fix on arm64), at the end of the day any user that wants to generate arm64 images (installer, rootfs, etc) on a x86 host will eventually need to run an arm64 container on x86.... |
That seems strange to me. Why should you have to run When I build images with upstream linuxkit - which works not too differently, it builds a tar stream of the target contents, then passes that tar stream through a container that generates the final output - it does it all natively, but can target any architecture. The first part is no big deal, since it is just copying contents from various OCI images into a tar stream. But the second part is fairly easy too. Did I understand you correctly? The reason you are running |
@deitch , I think I've got lost on this discussion 😆
Yes, the reason is explained here: #4592 IIRC my original fix was exactly add the TBH I don't understand why it worked before (without the |
Oh, now I see! I had forgotten about that one.
I don't know either. If these were pushed to a registry, or if run on docker with full multi-architecture support, all of this would go away. But as far as I know, it remains experimental.
This really does bother me. The entire contents of make-efi is 101 lines, almost a third is comments, and the vast majority of the rest is basic copy stuff. What in there would not work to cross-build? Can you give me a simple replication, how it gets used to build an arm64 ISO on x86_64 or vice-versa? This bothers me, and it is causing you issues. I want to see where the problem is and either fix it, or comment on it why it won't work. What is the most basic input I can run, e.g. after I already have an |
Sure, I think the easiest way to test is run the following on a x86 host:
It is going to fail on the first command.... Edit: I think this is actually the issue here.... we do have multi-platform container published to dockerhub, but when building it locally we do not export multi-architecture single container, instead, we export using My first fix is correct, since I didn't change the pkg/mkimage-iso-efi back them, it just fetched from dockerhub the multi-architecture container and it used (correctly) the arm64 version.... but now I'm changing the pkg/mkimage-iso-efi and then it's not published yet... and we don't have a single multi-architecture container published locally.... So now comes the question, what is the best way to address that? |
Ah, I missed your comment. The cat $INSTALLER_TAR | docker run -i --platform "linux/${ARCH}" --rm -e DEBUG="$DEBUG" -e VOLUME_LABEL=EVEISO -v "$ISO:/output.iso" "$MKIMAGE_TAG" $3 That So run it for amd64 on arm64, it works; same for arm64 on amd64, etc. cat $INSTALLER_TAR | docker run -i --rm -e DEBUG="$DEBUG" -e VOLUME_LABEL=EVEISO -v "$ISO:/output.iso" "$MKIMAGE_TAG" $3 I cannot tell you why that
Ah so that is the case. OK, I will try that. But I already can see: as long as the basic tag, i.e. We don't actually need multi-architecture anywhere. We need that when you call |
So do you run I tried that, ran it, it worked. Almost. I had to edit |
Since @rene didn't comment here but did explain to me in private channels, I will include his comment here for posterity. This is his discovery, credit where it is due. True, if [ ! -e boot/initrd.img ]; then
# all of the things we need to make a simple initrd
mkdir -p /tmp/initrd
(cd /tmp/initrd
mkdir -p bin lib sbin etc proc sys newroot
cp /initrd.sh init
cp /bin/busybox bin/
cp /lib/ld-musl* lib/
/bin/busybox --install -s /tmp/initrd/bin
find . | cpio -H newc -o | gzip > /tmp/initrd.img)
mv /tmp/initrd.img boot/initrd.img
fi It copies things from its running platform, specifically The |
There are a few ways to fix this:
As clean as the first one is, the real problem is not running I think we should do both. Let's get this fix in - given the messy constraints, @rene has the right answer, and did from the beginning - but let's fix |
I managed to fix This one still can do in, although once #4665 is in, we have the option to simplify |
Commit ee15f4d introduced changes to fetch the eve-mkimage-iso-efi container for the right target architecture, the fix was needed to generate ISO images for arm64. However, the ARCH prefix was missing for the linuxkit pkg tag resolution. This commit adds the ARCH prefix just as it's done in the tools/parse-pkgs.sh script.
PS: I just figured out this error now because I was changing the pkg/mkimage-iso-efi and it turns out the tag was missing.