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

rpm: dkms: Include other kernel-devel packages for spec requirements #16894

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

niclimcy
Copy link

@niclimcy niclimcy commented Dec 21, 2024

Motivation and Context

Description

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Copy link

@JMOdero JMOdero Dec 21, 2024

Choose a reason for hiding this comment

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

Are we sure about the use of "and" in lines 33 and 34? That didn't seem to work for me. I deleted those "ands" and just left one blank space between the "or" boolean statements.... Requires: (x or y or z) (a or b or c)

I didn't test the "Conflicts:" line.

@JMOdero
Copy link

JMOdero commented Dec 27, 2024

I haven't had time to test the "Conflicts:" line. As I previously mentioned openzfs compiled and installed fine without it on my system.

However, I'm wondering if something like this would be better if you wish to include it...

Conflicts: (kernel-devel and kernel-longterm-devel) (kernel-devel and kernel-16k-devel) (kernel-longterm-devel and kernel-16k-devel)

As far as "Conflicts:" goes I don't think we need to get into specific version ranges.

Conflicts: kernel-devel < @ZFS_META_KVER_MIN@, kernel-devel > @[email protected]
Requires: (kernel-devel >= @ZFS_META_KVER_MIN@ or kernel-longterm-devel >= @ZFS_META_KVER_MIN@ or kernel-16k-devel >= @ZFS_META_KVER_MIN@) and (kernel-devel <= @[email protected] or kernel-longterm-devel <= @[email protected] or kernel-16k-devel <= @[email protected])
Requires(post): (kernel-devel >= @ZFS_META_KVER_MIN@ or kernel-longterm-devel >= @ZFS_META_KVER_MIN@ or kernel-16k-devel >= @ZFS_META_KVER_MIN@) and (kernel-devel <= @[email protected] or kernel-longterm-devel <= @[email protected] or kernel-16k-devel <= @[email protected])
Conflicts: (kernel-devel >= @ZFS_META_KVER_MIN@ or kernel-longterm-devel >= @ZFS_META_KVER_MIN@ or kernel-16k-devel >= @ZFS_META_KVER_MIN@) and (kernel-devel <= @[email protected] or kernel-longterm-devel <= @[email protected] or kernel-16k-devel <= @[email protected])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Conflicts: (kernel-devel >= @ZFS_META_KVER_MIN@ or kernel-longterm-devel >= @ZFS_META_KVER_MIN@ or kernel-16k-devel >= @ZFS_META_KVER_MIN@) and (kernel-devel <= @[email protected] or kernel-longterm-devel <= @[email protected] or kernel-16k-devel <= @[email protected])
Conflicts: (kernel-devel < @ZFS_META_KVER_MIN@ or kernel-longterm-devel < @ZFS_META_KVER_MIN@ or kernel-16k-devel < @ZFS_META_KVER_MIN@) and (kernel-devel > @[email protected] or kernel-longterm-devel > @[email protected] or kernel-16k-devel > @[email protected])

Copy link
Contributor

Choose a reason for hiding this comment

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

You created a false conflict logic, which will not allow any valid kernel version.
Maybe that's why you are not able to build the rpm packages.

@AllKind
Copy link
Contributor

AllKind commented Dec 27, 2024

As far as "Conflicts:" goes I don't think we need to get into specific version ranges.

Of course the versions are needed. You cannot install onto a no longer, or not yet supported kernel version.

@JMOdero
Copy link

JMOdero commented Dec 28, 2024

As far as "Conflicts:" goes I don't think we need to get into specific version ranges.

Of course the versions are needed. You cannot install onto a no longer, or not yet supported kernel version.

Yes, but don't the "Requires:" and "Requires(post):" take care of that?

Like I said before the compile and install worked fine without the conflicts line, and as far as I know with my very limited knowledge openzfs never needed a "Conflicts:" line before in that spec file.

I'm new to all this so I could be very wrong, but isn't the "conflict" that we are concerned about at that stage, using two or more of kernel-devel, kernel-longterm-devel, or kernel-16k-devel when one of a different "flavor" is already in use?

So like my code suggests we are checking to see if we are using say kernel-16k-devel of ANY version and if so then kernel-devel of ANY version is a conflict. At that stage the version is not an issue. If say ANY kernal-longterm-devel is installed then we don't want to install kernel-devel nor kernel-16k-devel of ANY version.

Conflicts: (kernel-devel and kernel-longterm-devel) (kernel-devel and kernel-16k-devel) (kernel-longterm-devel and kernel-16k-devel)

My code suggestion is very clean and clear... Although it may not be correct. I haven't tested it.

Conflicts: (A and B) (A and C) (B and C)

If A and B are both present then the boolean is TRUE and there is a conflict.
If A and C are both present then the boolean is TRUE and there is a conflict.
If B and C are both present then the boolean is TRUE and there is a conflict.

The versions of A, B, and C do not matter at that stage. There is no version number that makes say having A and B at the same time okay.

Version numbers are handled in "Requires:" and "Requires(post):". Making sure we don't have two or more "flavors" of kernel-devel used at the same time seems to be addressed in "Conflicts:".

I think you guys are using too many "ands" and "ors." Looking at the rpm manual...

rpmrequires

My "Requires:" and "Requires:(post) are here. It has no "ands" nor "," it is cleaner, and most importantly it works.

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