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

Remove perl from installer image. #4594

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

deitch
Copy link
Contributor

@deitch deitch commented Feb 6, 2025

perl was part of PKGS, i.e. it was available both during build and during the final image. But perl is not needed in the final image. Removes it.

As a comparison, I created both images on x86_64 with docker build, then docker create, then docker export to see what the tarred up filesystem size is without compression:

131M    /tmp/installer-post.tar
165M    /tmp/installer-pre.tar

So this PR saves 34MB from the installer container and final installer image. In installer raw, it is squashfs, so it matters somewhat less, but for the ISO, this saved 34MB out of the ~800MB of the total, or just under 5%. Not bad for removing a single word from the Dockerfile.

perl was part of PKGS, i.e. it was available both during build and during the final image. But perl is not needed in the final image. Removes it.

Signed-off-by: Avi Deitcher <[email protected]>
@deitch deitch requested a review from eriknordmark as a code owner February 6, 2025 14:18
@deitch
Copy link
Contributor Author

deitch commented Feb 6, 2025

This does require a proper test.

@deitch
Copy link
Contributor Author

deitch commented Feb 6, 2025

Also, I checked for usage of perl in the installer container and in the single mounted file from mkimage-iso-efi, and no references there.

@deitch
Copy link
Contributor Author

deitch commented Feb 6, 2025

I am running an installer test right now. Will comment when done.

@OhmSpectator
Copy link
Member

This does require a proper test.

Are you planning to do it? =)

@deitch
Copy link
Contributor Author

deitch commented Feb 6, 2025

Are you planning to do it? =)

yeah, see above.

Copy link
Member

@OhmSpectator OhmSpectator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but @rucoder asks to perform a proper test of the installer before we merge it =)

@OhmSpectator
Copy link
Member

Are you planning to do it? =)

yeah, see above.

Got it, thanks!

@deitch
Copy link
Contributor Author

deitch commented Feb 6, 2025

Worked quite well on my installation test.

@deitch
Copy link
Contributor Author

deitch commented Feb 6, 2025

Why is get_run_id failing?

@OhmSpectator
Copy link
Member

OhmSpectator commented Feb 6, 2025

Why is get_run_id failing?

We have approved it too fast. The builds had not yet been completed. This action depends on completed builds.

@OhmSpectator
Copy link
Member

Worked quite well on my installation test.

I also see a confirmation from Zeljko that it works fine. Taking into account we do not have any installer-specific tests, I think we're good at merging it without waiting results of all the tests.

@OhmSpectator OhmSpectator merged commit 42c5315 into lf-edge:master Feb 6, 2025
22 of 23 checks passed
@deitch deitch deleted the remove-perl-from-installer branch February 6, 2025 15:51
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.

2 participants