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

Make Fedora dependencies Fedora-only #2081

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

jnovy
Copy link
Contributor

@jnovy jnovy commented Jul 8, 2024

No description provided.

Copy link

We were not able to find or create Copr project packit/containers-common-2081 specified in the config with the following error:

Cannot create a new Copr project (owner=packit project=containers-common-2081 chroots=['fedora-39-x86_64', 'fedora-eln-x86_64', 'fedora-40-x86_64', 'fedora-rawhide-x86_64']): Response is not in JSON format, there is probably a bug in the API code.

Please check your configuration for:

  1. typos in owner and project name (groups need to be prefixed with @)
  2. whether the project name doesn't contain not allowed characters (only letters, digits, underscores, dashes and dots must be used)
  3. whether the project itself exists (Packit creates projects only in its own namespace)
  4. whether Packit is allowed to build in your Copr project
  5. whether your Copr project/group is not private

@lsm5
Copy link
Member

lsm5 commented Jul 8, 2024

/packit build

Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

LGTM.

RHEL10 complained of not finding slirp4netns, and the qemu packages even if they were weak / conditional deps.

We should look at using an rpm macro for conditionalizing fedora-server deps but that's not a blocker here.

@rhatdan
Copy link
Member

rhatdan commented Jul 8, 2024

Needs DCO.

Requires: iptables
Requires: nftables
Requires: passt
%if %{defined fedora}
Conflicts: podman < 5:5.0.0~rc4-1
Recommends: composefs
Recommends: crun
Copy link
Member

Choose a reason for hiding this comment

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

This should be global?

Copy link
Member

Choose a reason for hiding this comment

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

The crun and netavark dependencies are already handled by the global:

Requires: oci-runtime
Requires: container-network-stack

so we should be fine with having crun and netavark inside Fedora, so people can use runc, cni if they want.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but the Recommends will pull in crun and netavark by default, while the oci-runtime and container-network-stack are potentially random. Having recommends does not prevent the use of cni or runc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rhatdan for RHEL10 runc and cni is gone - it is no longer available there.

Requires: iptables
Requires: nftables
Requires: passt
%if %{defined fedora}
Conflicts: podman < 5:5.0.0~rc4-1
Recommends: composefs
Recommends: crun
Requires: (crun if fedora-release-identity-server)
Requires: netavark >= %{netavark_epoch}:1.10.3-1
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@@ -85,19 +85,21 @@ Summary: Extra dependencies for Podman and Buildah
Requires: %{name} = %{epoch}:%{version}-%{release}
Requires: container-network-stack
Requires: oci-runtime
Requires: iptables
Copy link
Member

Choose a reason for hiding this comment

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

iptables dep should be dropped from RHEL/centos 10 as netavark must default to nftables. The iptables kernel modules were dropped anyway so it does not run correctly (https://issues.redhat.com/browse/RHEL-32374)

And thinking about this podman/containers-common does not use the nftables/iptables dep at all so shouldn't they be listed on netavark instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @Luap99 - it makes sense to maintain these types deps directly in the software requiring them. @lsm5 WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can drop nftables/iptables from the spec once a PR is merged to the netavark spec upstream first if you don't mind. I mean - I wouldn't block this PR for now but will do another one once netvark spec is updated upstream - if you are fine with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw. I just moved the iptables dep to Fedora section for now.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I am fine to not move it in this PR.
But I don't see the fedora only change pushed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad - just force pushed the change now.

Signed-off-by: Jindrich Novy <jnovy@redhat.com>
@lsm5
Copy link
Member

lsm5 commented Jul 9, 2024

LGTM

Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

If there are any further concerns about dep handling in RHEL10, we should be ok to address them in followup PRs.

@rhatdan
Copy link
Member

rhatdan commented Jul 9, 2024

/approve
/unhold

Copy link
Contributor

openshift-ci bot commented Jul 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jnovy, lsm5, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Jul 9, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit f123037 into containers:main Jul 9, 2024
14 checks passed
@jnovy jnovy deleted the extra-fix branch July 9, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants