@nhosoi @spetros

lgtm

Hello, I believe you should explain in a comment why you don't do the same change also for Fedora, since ansible-core exists also here? https://src.fedoraproject.org/rpms/ansible-core . It looks weird if on Fedora you BuildRequire ansible-core but not Require it, and on RHEL it is different. (I believe the answer is that on Fedora the Requires is added by the dependency generator in ansible-core: https://src.fedoraproject.org/rpms/ansible-core/blob/rawhide/f/ansible-generator#_30 so you don't have to add it manually, but can't you use the same mechanism in RHEL 9? If the RHEL 9 ansible-core package does not have the dependency generator, shouldn't it be added for consistency with the Fedora package?)

(And as you have already conditionally replaced ansible by ansible-core also in BuildRequires, you may build --with collection_artifact by default on RHEL. This way you will be able to build the collection artifact during the official builds instead of rebuilding the package in Copr using the EPEL Ansible. But this might be better to do in a separate PR.)

Hello, I believe you should explain in a comment

I'm assuming you mean a comment in the spec file.

why you don't do the same change also for Fedora, since ansible-core exists also here?

The problem is that there are two different packages in Fedora which provide the Ansible runtime commands such as ansible-playbook - both ansible and ansible-core.

https://fedoraproject.org/wiki/Changes/Ansible5

Do we want to force this choice on users of the linux-system-roles package?

On RHEL9, there is no choice - there is only ansible-core for RHEL users.

https://src.fedoraproject.org/rpms/ansible-core . It looks weird if on Fedora you BuildRequire ansible-core but not Require it, and on RHEL it is different. (I believe the answer is that on Fedora the Requires is added by the dependency generator in ansible-core: https://src.fedoraproject.org/rpms/ansible-core/blob/rawhide/f/ansible-generator#_30 so you don't have to add it manually, but can't you use the same mechanism in RHEL 9? If the RHEL 9 ansible-core package does not have the dependency generator, shouldn't it be added for consistency with the Fedora package?)

(And as you have already conditionally replaced ansible by ansible-core also in BuildRequires, you may build --with collection_artifact by default on RHEL. This way you will be able to build the collection artifact during the official builds instead of rebuilding the package in Copr using the EPEL Ansible. But this might be better to do in a separate PR.)

Right.

Hello, I believe you should explain in a comment

I'm assuming you mean a comment in the spec file.

yes, sure. It is not obvious why it is so and why there is a difference between RHEL and Fedora, so it deserves a comment.

why you don't do the same change also for Fedora, since ansible-core exists also here?

The problem is that there are two different packages in Fedora which provide the Ansible runtime commands such as ansible-playbook - both ansible and ansible-core.

https://fedoraproject.org/wiki/Changes/Ansible5

Do we want to force this choice on users of the linux-system-roles package?

Maybe I don't understand ... By "forcing the choice" you mean making sure that both ansible-core and ansible satisfy the requirement? If so, and if both of them actually satisfy the package's needs (and I assume they do, since otherwise it would not work on RHEL 9 either), what's wrong with that?
Anyway, the Requires on either ansible or ansible-core is already there. Check your scratch build. (This was added by the dependency generator, as I mention below, and is actually a valid reason for making this Requires conditional for RHEL 9, as I also mention below.)

On RHEL9, there is no choice - there is only ansible-core for RHEL users.

https://src.fedoraproject.org/rpms/ansible-core . It looks weird if on Fedora you BuildRequire ansible-core but not Require it, and on RHEL it is different. (I believe the answer is that on Fedora the Requires is added by the dependency generator in ansible-core: https://src.fedoraproject.org/rpms/ansible-core/blob/rawhide/f/ansible-generator#_30 so you don't have to add it manually, but can't you use the same mechanism in RHEL 9? If the RHEL 9 ansible-core package does not have the dependency generator, shouldn't it be added for consistency with the Fedora package?)

(And as you have already conditionally replaced ansible by ansible-core also in BuildRequires, you may build --with collection_artifact by default on RHEL. This way you will be able to build the collection artifact during the official builds instead of rebuilding the package in Copr using the EPEL Ansible. But this might be better to do in a separate PR.)

Right.

By the way, we tested this with @spetros last week, it worked fine.

By "forcing the choice" you mean making sure that both ansible-core and ansible satisfy the requirement?

Now when rereading I understood what you meant - forcing them to use ansible-core and not ansible. The right way would be to require either ansible or ansible-core, which can be expressed using boolean dependencies. Maybe you could do it even for RHEL - is there a problem with requiring (ansible-core or ansible)? (The target user would be the one who wants to install ansible from EPEL instead of ansible-core from RHEL - if there's going to be ansible in EPEL 9.)

By "forcing the choice" you mean making sure that both ansible-core and ansible satisfy the requirement?

Now when rereading I understood what you meant - forcing them to use ansible-core and not ansible. The right way would be to require either ansible or ansible-core, which can be expressed using boolean dependencies.

How do you do this? Something like

%if %{with ansible-core}
Requires: ansible-core
%else
Requires: ansible
%endif

but how does the user specify this e.g. in the dnf command line? dnf install rhel-system-roles --with ansible?

Maybe you could do it even for RHEL - is there a problem with requiring (ansible-core or ansible)? (The target user would be the one who wants to install ansible from EPEL instead of ansible-core from RHEL - if there's going to be ansible in EPEL 9.)

By "forcing the choice" you mean making sure that both ansible-core and ansible satisfy the requirement?

Now when rereading I understood what you meant - forcing them to use ansible-core and not ansible. The right way would be to require either ansible or ansible-core, which can be expressed using boolean dependencies.

How do you do this? Something like
%if %{with ansible-core} Requires: ansible-core %else Requires: ansible %endif

Not at all, the Requires syntax allows this itself:

Requires: (ansible >= 2.9.0 or ansible-core >= 2.11.0)

and, if you check the Requires on your Fedora binary package, it is already there, so you need to do it only for RHEL.
Actually, what bothers me the most about this change is that the reasoning behind adding the dependency only on RHEL and not on Fedora is missing. Anyone reading the spec will likely ask why does this difference between RHEL and Fedora exists.

By "forcing the choice" you mean making sure that both ansible-core and ansible satisfy the requirement?

Now when rereading I understood what you meant - forcing them to use ansible-core and not ansible. The right way would be to require either ansible or ansible-core, which can be expressed using boolean dependencies.

How do you do this? Something like
%if %{with ansible-core} Requires: ansible-core %else Requires: ansible %endif

Not at all, the Requires syntax allows this itself:
Requires: (ansible >= 2.9.0 or ansible-core >= 2.11.0)
and, if you check the Requires on your Fedora binary package, it is already there, so you need to do it only for RHEL.

Indeed it is:

> rpm -qp --requires results_linux-system-roles/1.11.0/2.fc36/linux-system-roles-1.11.0-2.fc36.noarch.rpm 
(ansible >= 2.9.0 or ansible-core >= 2.11.0)

I am not familiar with this syntax. How does it work? Where does it come from? I don't see a Requires: for this. There is the

%if %{with ansible}
BuildRequires: %{ansible_build_dep}
%endif

but I don't understand how that translates to a Requires: (ansible >= 2.9.0 or ansible-core >= 2.11.0)

Actually, what bothers me the most about this change is that the reasoning behind adding the dependency only on RHEL and not on Fedora is missing. Anyone reading the spec will likely ask why does this difference between RHEL and Fedora exists.

Well, since I now know that there is a way to have a boolean dependency, that is the best way to do this.

Do you know if this works on both el8 and el7?

By "forcing the choice" you mean making sure that both ansible-core and ansible satisfy the requirement?

Now when rereading I understood what you meant - forcing them to use ansible-core and not ansible. The right way would be to require either ansible or ansible-core, which can be expressed using boolean dependencies.

How do you do this? Something like
%if %{with ansible-core} Requires: ansible-core %else Requires: ansible %endif

Not at all, the Requires syntax allows this itself:
Requires: (ansible >= 2.9.0 or ansible-core >= 2.11.0)
and, if you check the Requires on your Fedora binary package, it is already there, so you need to do it only for RHEL.

Indeed it is:
rpm -qp --requires results_linux-system-roles/1.11.0/2.fc36/linux-system-roles-1.11.0-2.fc36.noarch.rpm (ansible >= 2.9.0 or ansible-core >= 2.11.0)
I am not familiar with this syntax. How does it work?

https://rpm-software-management.github.io/rpm/manual/boolean_dependencies.html

Where does it come from? I don't see a Requires: for this. There is the
%if %{with ansible} BuildRequires: %{ansible_build_dep} %endif
but I don't understand how that translates to a Requires: (ansible >= 2.9.0 or ansible-core >= 2.11.0)

It does not! The Requires: comes from a different place: the Ansible dependency generator. https://src.fedoraproject.org/rpms/ansible-core/blob/rawhide/f/ansible-generator#_30
If the generator gets added to the RHEL ansible(-core) packages (currently it is only in the Fedora package), you can drop the dependency line entirely.

Actually, what bothers me the most about this change is that the reasoning behind adding the dependency only on RHEL and not on Fedora is missing. Anyone reading the spec will likely ask why does this difference between RHEL and Fedora exists.

Well, since I now know that there is a way to have a boolean dependency, that is the best way to do this.

Do you know if this works on both el8 and el7?

el8 yes, el7 AFAIK no.

el7 https://bugzilla.redhat.com/show_bug.cgi?id=1479531

can we do something like this:

%if 0%{?rhel} >= 8 # assuming ansible-core in rhel 8.6
Requires: (ansible-core >= 2.11.0 or ansible >= 2.9.0)
%endif

Fedora - the automatic generator will add this - no need to explicit declare it in the spec file
EL7 - no dependency on ansible because there is no ansible in el7 - user is responsible for knowing they have to install ansible

Also - for rhel - I would like the default to be ansible-core - if the user just does dnf install rhel-system-roles it should install ansible-core if they don't do anything, or have ansible already installed

@pcahyna how's this? https://src.fedoraproject.org/rpms/linux-system-roles/pull-request/48#request_diff

You should say why is this line RHEL-specific, because it is not obvious. Something like # on Fedora, the ansible dependency generator adds this dependency automatically, may revisit this if/when the generator gets added to the RHEL ansible-core package