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

Override releasever_{major,minor} with system-release provides #2198

Conversation

evan-goode
Copy link
Member

@evan-goode evan-goode commented Jan 21, 2025

The releasever_major and releasever_minor substitution variables are usually derived by splitting releasever on the first .. However, to support EPEL 10 [1], we would like a way for distributions to override these values. Specifically, we would like RHEL 10 to have a releasever of 10 with a releasever_major of 10 and a releasever_minor of 0 (later incrementing to 1, 2, to correspond with the RHEL minor version).

This commit adds a new API function, detect_releasevers, which derives releasever, releasever_major, and releasever_minor from virtual provides on the system-release package (any of DISTROVERPKG). The detection of releasever is unchanged. releasever_major and releasever_minor are specified by the versions of the system-release-major and system-release-minor provides, respectively.

If the user specifies a --releasever=X.Y on the command line, the distribution settings for releasever, releasever_major, and releasever_minor will all be overridden: releasever will be set to X.Y, releasever_major will be set to X, and releasever_minor will be set to Y, same as before.

If a user wants to specify a custom releasever_{major,minor}, they have to set all three with --setopt=releasever=X --setopt=releasever_major=Y --setopt=releasever_minor=z, taking care to put releasever_major and releasever_minor after releasever so they are not overridden. This is admittedly not ideal, but I can't think of another solution to this problem that preserves the following properties:

  1. If they are not overridden, releasever_{major,minor} are derived by splitting releasever. This behavior was added in Split $releasever to $releasever_major and $releasever_minor #1989.
  2. If --releasever is specified on the command line, releasever_{major,minor} should be set accordingly, even if releasever{major_minor} are specified by provides in the distribution system-release package.
  3. The user should somehow be able to specify their own custom releasever, releasever_major, and releasever_minor.

Maybe we could add --releasever_major= and --releasever_minor options that take priority over --releasever to improve point (3).

Another caveat to allowing overriding releasever_{major,minor} is existing API users using detect_releasever. Take this snippet from our doc/examples/install_extension.py:

    with dnf.Base() as base:
        # Substitutions are needed for correct interpretation of repo files.
        RELEASEVER = dnf.rpm.detect_releasever(base.conf.installroot)
        base.conf.substitutions['releasever'] = RELEASEVER

Now, in order to detect the correct releasever_{major,minor}, the correct code would be

    with dnf.Base() as base:
        # Substitutions are needed for correct interpretation of repo files.
        RELEASEVER, MAJOR, MINOR = dnf.rpm.detect_releasevers(base.conf.installroot)
        base.conf.substitutions['releasever'] = RELEASEVER
        if MAJOR is not None:
            base.conf.substitutions['releasever_major']
        if MINOR is not None:
            base.conf.substitutions['releasever_minor']

Requires rpm-software-management/libdnf#1689.

[1] https://issues.redhat.com/browse/RHEL-68034

dnf/const.py.in Outdated Show resolved Hide resolved
@evan-goode evan-goode force-pushed the evan-goode/provide-releasever-major-minor branch from 1a54f5b to 4872d0a Compare January 22, 2025 18:55
@jan-kolarik
Copy link
Member

Hi Evan and thanks for figuring this out! For me it sounds fine having new --releasever_major and --releasever_minor options that will take priority over --releasever + adding new detect_releasevers method. Just make sure to clearly document that existing use cases are preserved and how the new API should be used and why we are introducing it.

@evan-goode evan-goode force-pushed the evan-goode/provide-releasever-major-minor branch from fa3f84f to 0758ec7 Compare January 27, 2025 19:47
@evan-goode evan-goode marked this pull request as ready for review January 27, 2025 19:47
@evan-goode
Copy link
Member Author

ci-dnf-stack PR: rpm-software-management/ci-dnf-stack#1621

@evan-goode
Copy link
Member Author

Hi Evan and thanks for figuring this out! For me it sounds fine having new --releasever_major and --releasever_minor options that will take priority over --releasever + adding new detect_releasevers method. Just make sure to clearly document that existing use cases are preserved and how the new API should be used and why we are introducing it.

Cool, I've added the --releasever-major and --releasever-minor command-line options for overriding the major,minor variables, and I documented detect_releasevers in api_rpm.rst.

@evan-goode
Copy link
Member Author

Tests are passing locally with rpm-software-management/libdnf#1689 and rpm-software-management/ci-dnf-stack#1621

Copy link
Contributor

@kontura kontura left a comment

Choose a reason for hiding this comment

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

To be honest I find the requested behavior awful.
I would expect that releasever == releasever_major.releasever_minor is always true.

dnf/conf/substitutions.py Outdated Show resolved Hide resolved
dnf/conf/config.py Show resolved Hide resolved
dnf/conf/substitutions.py Outdated Show resolved Hide resolved
tests/test_config.py Show resolved Hide resolved
doc/command_ref.rst Outdated Show resolved Hide resolved
@evan-goode
Copy link
Member Author

To be honest I find the requested behavior awful. I would expect that releasever == releasever_major.releasever_minor is always true.

I agree, it's gross. I would have preferred changing the RHEL releasever to 10.0 to be consistent with CentOS Stream, but that was shot down here: https://issues.redhat.com/browse/RHELBLD-16135.

@carlwgeorge mentioned an "emergency backup plan" if we could not make this DNF change in time or had strong feelings against it. IIRC, that solution involves a scriptlet on epel-release that creates /etc/dnf/vars/epelrelease with the correct EPEL release version. @carlwgeorge would you care to explain why these DNF changes would be preferred?

@evan-goode evan-goode force-pushed the evan-goode/provide-releasever-major-minor branch from b352c77 to 99c2351 Compare February 5, 2025 23:31
@Conan-Kudo Conan-Kudo self-assigned this Feb 6, 2025
@Conan-Kudo
Copy link
Member

To be honest I find the requested behavior awful. I would expect that releasever == releasever_major.releasever_minor is always true.

@kontura I agree with you on this, I would actually prefer that everything else correctly adapt to this paradigm, since it can be useful in a variety of ways. That said, it's not really the end of the world to have override knobs. I still don't understand why subscription-manager or rhc cannot handle this properly.

@carlwgeorge mentioned an "emergency backup plan" if we could not make this DNF change in time or had strong feelings against it. IIRC, that solution involves a scriptlet on epel-release that creates /etc/dnf/vars/epelrelease with the correct EPEL release version. @carlwgeorge would you care to explain why these DNF changes would be preferred?

@evan-goode I'm not Carl, but I can answer this question. Requiring custom DNF variables makes integration very difficult for a variety of cases. These two reasons are why we want to avoid this:

  • It's completely unsupported in all image build tools. KIWI, OSBuild, Lorax, mkosi, and LiveCD Creator all cannot process or propagate custom DNF variables, so it becomes quite difficult to integrate EPEL into image building processes in a natural fashion. The releasever value is exposed and can be propagated as a controlled variable through command line arguments, but everything else is not possible due to clean roots and sanitizing environment variables. This makes life pretty miserable for downstreams (e.g. CentOS Hyperscale, HeliumOS, etc.) I also doubt custom DNF variables are supported in RPM-OSTree, as they are not used in Fedora at all, so it would have never occurred to anyone to wire them up in RPM-OSTree's custom stuff.
  • Relying on scriptlets can be iffy as things like RPM-OSTree and other tools attempt to restrict or sanitize the execution domain of scriptlets in packages. The main casualty is the implicit restriction on Lua scriptlets, as they are unimplemented in RPM-OSTree, but there are other things that go on that make scriptlet behavior unpredictable, since RPM-OSTree reimplements rpm-install behavior quite differently.

@carlwgeorge
Copy link
Contributor

@carlwgeorge mentioned an "emergency backup plan" if we could not make this DNF change in time or had strong feelings against it. IIRC, that solution involves a scriptlet on epel-release that creates /etc/dnf/vars/epelrelease with the correct EPEL release version.

Correct. We previously prototyped this for epel9. I just put up a draft PR to do this for epel10, mainly to get a scratch build for testing.

@carlwgeorge would you care to explain why these DNF changes would be preferred?

With the scriptlet approach, we would be writing a file that is arguably DNF configuration during a DNF transaction. I'm concerned about what side effects that can have, or if it's a safe approach at all. It does appear to work fine in limited testing. If you're not concerned about this as a DNF developer, then I would feel much better about it.

Another slight concern is that we'll be running the scriptlet on every dnf transaction that has a package with a file in /etc, because the %transfiletriggerin scriptlet only accepts directories as arguments and we need to source the value from /etc/os-release. For transactions that don't involve the /etc/os-release file this just means running a single grep command, so it's probably not a big deal, I just wish we could target the scriptlet to specific files as opposed to a whole directory to be more efficient.

I was initially concerned about the impact of using a file trigger scriptlet with rpm-ostree, but with the speedy adoption of bootc and the fact that this will on be needed on EL10, I'm not sure we need to worry about it anymore.

@carlwgeorge
Copy link
Contributor

It's completely unsupported in all image build tools. KIWI, OSBuild, Lorax, mkosi, and LiveCD Creator all cannot process or propagate custom DNF variables, so it becomes quite difficult to integrate EPEL into image building processes in a natural fashion.

How do these tools handle the $stream variable that is already in use for metalinks/baseurls in CentOS Stream?

@Conan-Kudo
Copy link
Member

It's completely unsupported in all image build tools. KIWI, OSBuild, Lorax, mkosi, and LiveCD Creator all cannot process or propagate custom DNF variables, so it becomes quite difficult to integrate EPEL into image building processes in a natural fashion.

How do these tools handle the $stream variable that is already in use for metalinks/baseurls in CentOS Stream?

They don't. At least in the cases where repo files aren't used directly, new repo definitions are used with ${releasever}-stream instead. Otherwise some kind of forking and editing is done to change them.

@Conan-Kudo
Copy link
Member

Another slight concern is that we'll be running the scriptlet on every dnf transaction that has a package with a file in /etc, because the %transfiletriggerin scriptlet only accepts directories as arguments and we need to source the value from /etc/os-release. For transactions that don't involve the /etc/os-release file this just means running a single grep command, so it's probably not a big deal, I just wish we could target the scriptlet to specific files as opposed to a whole directory to be more efficient.

If it targeted the file, it would never fire, since /etc/os-release is a symlink that never changes.

@carlwgeorge
Copy link
Contributor

File trigger prefix matching explicitly includes symlinks, and doesn't check if the matching file has been modified, only that it's part of the transaction (or already exists on the system in the case of the package with the trigger being installed). But it's a moot point since it only works with directories.

@kontura
Copy link
Contributor

kontura commented Feb 6, 2025

With the scriptlet approach, we would be writing a file that is arguably DNF configuration during a DNF transaction. I'm concerned about what side effects that can have, or if it's a safe approach at all. It does appear to work fine in limited testing. If you're not concerned about this as a DNF developer, then I would feel much better about it.

I personally wouldn't worry about creating a file in /etc/dnf/vars during a transaction.
However based on the previous @Conan-Kudo comment I gather we need to do this anyway.

The changes look good to me now but I am confused by the failing Packit builds.

@kontura
Copy link
Contributor

kontura commented Feb 6, 2025

It does fail reliably in COPR builds, I have bisected it and it seems to be caused by 90604c3.

@evan-goode
Copy link
Member Author

It does fail reliably in COPR builds, I have bisected it and it seems to be caused by 90604c3.

I think it's the use of ConfigParser.splitReleasever. The Copr environment must be missing rpm-software-management/libdnf#1689 for some reason.

@evan-goode
Copy link
Member Author

We'll need to bump the libdnf version for the new API function and Require it from dnf.

This allows setting a releasever_major or releasever_minor
independent of releasever, which is needed by EPEL.

Related: https://issues.redhat.com/browse/RHEL-68034
The releasever_major and releasever_minor substitution variables are
usually derived by splitting releasever on the first `.`. However, to
support EPEL 10 [1], we would like a way for distributions to override these
values. Specifically, we would like RHEL 10 to have a releasever of `10`
with a releasever_major of `10` and a releasever_minor of `0` (later
incrementing to `1`, `2`, to correspond with the RHEL minor version).

This commit adds a new API function, `detect_releasevers`, which derives
releasever, releasever_major, and releasever_minor from virtual provides
on the system-release package (any of `DISTROVERPKG`). The detection of
releasever is unchanged. releasever_major and releasever_minor are
specified by the versions of the `system-release-major` and
`system-release-minor` provides, respectively.

If the user specifies a `--releasever=X.Y` on the command line, the
distribution settings for releasever, releasever_major, and releasever_minor
will all be overridden: releasever will be set to X.Y, releasever_major will be
set to X, and releasever_minor will be set to Y, same as before.  If a user
wants to specify a custom releasever_major and releasever_minor, they have to
set all three with `--setopt=releasever=X --setopt=releasever_major=Y
--setopt=releasever_minor=z`, taking care to put `releasever_major` and
`releasever_minor` after `releasever` so they are not overridden.

[1] https://issues.redhat.com/browse/RHEL-68034
Allows the user to override the $releasever_major and $releasever_minor
variables on the command line, like --releasever.
Adds dnf.rpm.detect_releasevers to the API docs and mention it is
now preferred over dnf.rpm.detect_releasever.

Updates examples/install_extension.py to use detect_releasevers and set
the releasever_major and releasever_minor substitution variables.
@evan-goode evan-goode force-pushed the evan-goode/provide-releasever-major-minor branch from 99c2351 to 4c37ce8 Compare February 6, 2025 18:34
@evan-goode
Copy link
Member Author

We'll need to bump the libdnf version for the new API function and Require it from dnf.

libdnf already got a version bump to 0.74.0 after this change, and dnf master now Requires it. So I rebased this to dnf master.

@kontura kontura merged commit e931960 into rpm-software-management:master Feb 7, 2025
3 of 11 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.

5 participants