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

Run test-suite environment setup inside the image #3531

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

pmatilai
Copy link
Member

Commit c17cc69 introduced an rpmdb --initdb call into test-suite make_install() but at that point, it's actually the rpm from the host, which is NOT what we want.

Split the image preparation to two stages: install, which runs from the outside and basically just copies data into the image, and setup which runs from inside the image. For this we need some extra container gymnastics.

Fixes: #3530

@pmatilai pmatilai requested a review from dmnks January 23, 2025 08:37
@pmatilai pmatilai requested a review from a team as a code owner January 23, 2025 08:37
@pmatilai pmatilai requested review from ffesti and removed request for a team January 23, 2025 08:37
Commit c17cc69 introduced an
`rpmdb --initdb` call into test-suite make_install() but at that point,
it's actually the rpm from the host, which is NOT what we want.

Split the image preparation to two stages: install, which runs from the
outside and basically just copies data into the image, and setup which
runs from inside the image. For this we need some extra container
gymnastics.

Fixes: rpm-software-management#3530
@pmatilai
Copy link
Member Author

Oops, some debugging leftovers in there... (fixed)

@dmnks
Copy link
Contributor

dmnks commented Jan 23, 2025

I think there are two ways to do the split:

  1. Based on where the script is run (host vs. container)
  2. Based on what the script does (installs rpm vs. sets up the image and test-suite)

This patch is a blend of both, i.e. the split seems a bit arbitrary 😅 (Of course, the most important part is the rpmdb commands which are now run inside the container, so that's good.)

That said, we can always improve this later so it's not a blocker.

@pmatilai
Copy link
Member Author

pmatilai commented Jan 23, 2025

Right, the split-line is a bit fuzzy but basically, only do on the outside what has to be on the outside: copying data into the image. Beyond that whats install and what's setup gets increasingly fuzzy.

I left the ldconfig stuff alone because it's kinda part of the "cmake install" step, but could certainly be moved to setup as well. If rpm shipped it's own ld.so.conf.d file, putting it into place would be necessarily part of install. But since we create it here, it can be either 😅

@pmatilai
Copy link
Member Author

Hmm, but that said: ldconfig on the host might not be compatible with what's on the inside. So while putting the conf file in place can be considered install, running ldconfig should actually happen inside.

@dmnks
Copy link
Contributor

dmnks commented Jan 23, 2025

... but basically, only do on the outside what has to be on the outside: copying data into the image.

Oh, indeed. You're right, I didn't realize this initially. We are copying stuff from the host, which is one way to draw the line, and I agree it's probably the most sensible 😄 Thanks for clarifying!

I left the ldconfig stuff alone because it's kinda part of the "cmake install" step, but could certainly be moved to setup as well. If rpm shipped it's own ld.so.conf.d file, putting it into place would be necessarily part of install. But since we create it here, it can be either 😅
[...]
Hmm, but that said: ldconfig on the host might not be compatible with what's on the inside. So while putting the conf file in place can be considered install, running ldconfig should actually happen inside.

Yep, the ldconfig stuff wasn't necessarily what I had in mind, it was more about the various directories and whatnot that we copy/create, but I stand corrected, see above 😄

@dmnks
Copy link
Contributor

dmnks commented Jan 23, 2025

OK, looks good otherwise, thanks!

@dmnks dmnks merged commit add3e06 into rpm-software-management:master Jan 23, 2025
1 check passed
@pmatilai
Copy link
Member Author

Oh, I was about to move running ldconfig into setup because it is actually wrong on the install side, but there's always the next PR 😄

@dmnks
Copy link
Contributor

dmnks commented Jan 23, 2025

Oops, my merge-this finger was itching too much I guess 😄 But yup, there's always the next one.

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.

Separate test-suite install and setup stages
2 participants