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 shadow-utils inside the chroot to avoid contradictory configurations #1283

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

mhjacks
Copy link

@mhjacks mhjacks commented Jan 10, 2024

This is the suggestion that @sgallagher offered; I tested this on a FreeIPA-enrolled system and it worked.
I'm a bit nervous about the comment above the line about old shadow-utils being problematic; but we definitely also have the case where shadow-utils config from the host "leaks" into the chroot. I'm not sure which problem is bigger.

@sgallagher
Copy link
Contributor

FWIW, I think the comment there refers mostly to whether the version of shadow-utils in the container supported the --root option, which it probably didn't if we were building for an old RHEL release. I'm 99% sure that all non-EOL RHEL systems have that feature at this point.

@mhjacks
Copy link
Author

mhjacks commented Jan 10, 2024

That's good to know. This approach specifically avoids using the --root option now because of the "leaking" problem discussed above. Thanks for the feedback!

@mhjacks
Copy link
Author

mhjacks commented Jan 10, 2024

This is pursuant to our Slack discussion of how to handle this - I know the full patch would need documentation in the defaults file and probably tests as well. Since the buildroot object has the config dict, we can look it up there.

The way this is structured, the default case would be to use the host's shadow-utils, and then only if the explicit configuration is set to use the chroot's would the chroot's utils be used. I'm definitely open to more idiomatic ways to express it but that's the intention.

@praiskup
Copy link
Member

Thank you for your work on this, @mhjacks.

Note that Mock no longer installs shadow-utils into the minimal buildroot as it used to, this has changed in a34d3ae. Mock never installed shadow-utils into the bootstrap chroot explicitly.
bootstrap typically only contains dnf and its dependencies. But since this is opt-in, I think it is just fine to depend on an implicit set of packages.

mock/py/mockbuild/shadow_utils.py Fixed Show resolved Hide resolved
@praiskup
Copy link
Member

FWIW, I think the comment there refers mostly to whether the version of shadow-utils in the container supported the --root option, which it probably didn't if we were building for an old RHEL release. I'm 99% sure that all non-EOL RHEL systems have that feature at this point.

There only used to be nuance between -N and -n option, see the 4.0 release notes. But indeed, this is no longer the case for non-EOL RHEL systems.

Mock never used --prefix with shadow-utils from bootstrap, and never used ShadoUtils' --root at all. That's because --root seems to be broken even on recent distributions.

The reasoning behind the "on host decision" is that Mock upstream expects that the machines that are used by users for Mock builds are "new enough". Ideally Fedora builders, or the latest RHEL releases (users should start moving builders out from RHEL8 definitely). Then, using the tooling "on host" is much more predictable (and fixable) for the builder machine administrator) then relying on various in-chroot tooling versions (one machine typically builds for different distributions, and builder admins are unlikely to have rights to fix the target buildroots).

@mhjacks mhjacks marked this pull request as draft January 11, 2024 14:25
@mhjacks
Copy link
Author

mhjacks commented Jan 11, 2024

Converting to draft to add the other items you requested. Thanks for the review!

@mhjacks mhjacks marked this pull request as ready for review January 11, 2024 15:04
Copy link
Member

@praiskup praiskup left a comment

Choose a reason for hiding this comment

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

Thank you!

@praiskup praiskup merged commit 4b3fb22 into rpm-software-management:main Jan 15, 2024
16 checks passed
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