Skip to content

Conversation

@jnovy
Copy link
Contributor

@jnovy jnovy commented Dec 15, 2025

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

  • Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
    commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
    the sign-off email address. See CONTRIBUTING.md
    for more information.
  • Referenced issues using Fixes: #00000 in commit message (if applicable)
  • Tests have been added/updated (or no tests are needed)
  • Documentation has been updated (or no documentation changes are needed)
  • All commits pass make validatepr (format/lint checks)
  • Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

None

rpm/podman.spec Outdated
%ifarch %{machine_arches}
# symlink virtiofsd in %%{name} libexecdir for machine subpackage
ln -s ../virtiofsd %{buildroot}%{_libexecdir}/%{name}
%if %{defined rhel}
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be replaced with %if %{defined qemu} itself. See the qemu conditional at the top of spec file.

Same for the %files change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lsm5 I don't think %if %{defined qemu} would work correctly here. Looking at the spec file, the qemu macro is defined on Fedora (line 15, inside the %if %{defined fedora} block), but this symlink is specifically needed on RHEL where qemu-kvm is used instead of the qemu-system-* packages.

Using %if %{defined qemu} would create the symlink on Fedora (where it's not needed since qemu-system-%{arch} already exists), while skipping it on RHEL (where it's actually needed).

I think %if %{defined rhel} is more explicit and clearer here - it directly expresses the intent that this symlink is a RHEL-specific workaround for the different qemu package naming. It also makes the code more maintainable if the qemu macro definition logic changes in the future.

Unless I'm misunderstanding something about the qemu macro usage?

Copy link
Member

Choose a reason for hiding this comment

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

my bad. Should be %if !%{defined qemu} for RHEL envs. If the qemu itself sounds confusing, we could switch to some other symbol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lsm5 You're right, I'll use %if !%{defined qemu} to stay consistent with the existing pattern in the spec.

However, I'd like to raise a broader concern: the increasing use of macro indirection in this spec makes it harder to port and maintain, especially for RHEL-specific work. When I see %if !%{defined qemu}, I have to:

  1. Search for where qemu is defined
  2. Find it's defined only on Fedora (line 15)
  3. Apply inverse logic to understand this means RHEL/CentOS Stream
  4. Check the comment to understand it's about package naming differences

For someone maintaining the RHEL spec or doing distro-specific work, %if %{defined rhel} would be immediately clear without requiring this detective work.

I'll make this change for consistency, but I'd like to propose we consider re-engineering the spec to reduce these kinds of indirections and use more explicit distro conditionals (%{defined rhel}, %{defined fedora}) for platform-specific sections.

This would make the spec more self-documenting and easier to maintain across distributions. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

There are also cases when conditionals don't apply across all fedora or all rhel, and in those cases stating out those same conditionals in multiple places can become unmanageable.

Having the indirection using a macro / symbol / whatever specified at the top makes the justification about the environment clear and reusable IMO. This is useful when we're using a single spec file to build for multiple distro-release-arch combinations as is the case in rpm/podman.spec .

If %if !%{defined qemu} can be hard to read, I dunno, maybe %if "%qemu" == "0" or something, where qemu is defined for all envs and set to 1 or 0 depending on what env supports it ??

Copy link
Member

Choose a reason for hiding this comment

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

btw, what's the harm in copying the conditionals specified on top and the other changes from upstream / fedora spec file to rhel? Is there a case where it'll fail to build or build the wrong way?

@lsm5 lsm5 added the No New Tests Allow PR to proceed without adding regression tests label Dec 15, 2025
@jnovy jnovy force-pushed the specfix-symlink-rhel branch from ccdb36b to c186a33 Compare December 15, 2025 16:19
@packit-as-a-service
Copy link

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

@mheon
Copy link
Member

mheon commented Dec 15, 2025

Why are we doing this? I assume something related to machine packaging, but what does this solve?

@jakecorrenti
Copy link
Member

jakecorrenti commented Dec 15, 2025

Why are we doing this? I assume something related to machine packaging, but what does this solve?

If the user is on a RHEL host, podman machine currently only searches for the qemu-system-* binary, which wouldn't exist (just qemu-kvm). It's related to RUN-3579

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No New Tests Allow PR to proceed without adding regression tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants