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

podman.spec: recommend slirp4netns #1271

Merged
merged 1 commit into from
Aug 16, 2018

Conversation

vrothberg
Copy link
Member

Fixes: #1234
Signed-off-by: Valentin Rothberg [email protected]

@vrothberg
Copy link
Member Author

@rhatdan PTAL. I picked up the low hanging fruit from #1234 (comment). As slirp4netns is already packaged for Fedora, I assume the change is okay.

@@ -69,6 +69,7 @@ Recommends: container-selinux
%else
Requires: container-selinux
%endif
Recommends: slirp4netns
Copy link
Member

Choose a reason for hiding this comment

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

Can you put this into the same block above. So on RHEL it Requires else it recommends.

Copy link

@Conan-Kudo Conan-Kudo Aug 15, 2018

Choose a reason for hiding this comment

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

It doesn't exist in RHEL, so it should just be conditioned for "not-RHEL", since Fedora, SLE, openSUSE, and others all support weak dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Well it will exist in RHEL8. But I am fine with just moving it up with the ifdef block.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the line in the if branch, so it'll affect Fedora. Once it's packaged in RHEL, it can be added to the else branch.

@giuseppe
Copy link
Member

thanks for working on this. LGTM

@vrothberg vrothberg force-pushed the recommend-slirp4netns branch from 235ae8a to 6b493ee Compare August 15, 2018 12:15
@rhatdan
Copy link
Member

rhatdan commented Aug 15, 2018

Does SUSE support Recommends yet?

@vrothberg
Copy link
Member Author

Does SUSE support Recommends yet?

It does but we're not using this .spec for packaging.

@Conan-Kudo
Copy link

Perhaps as a separate PR, we should consider enhancing the packaging so that it can be tested in COPR against Fedora, EL7, openSUSE Leap 15.0, and openSUSE Tumbleweed.

@rhatdan
Copy link
Member

rhatdan commented Aug 15, 2018

Yes that was what I was thinking. The goal of the spec, I believe it to allow random people to use it for their distribution.

We don't use this spec for Fedora or RHEL or Centos, but we use something close to it. (I wish it was closer.)

@rhatdan
Copy link
Member

rhatdan commented Aug 15, 2018

THis should be

%if 0%{?fedora}
Recommends: container-selinux
Recommends: slirp4netns
%else
Requires: container-selinux
Requires: slirp4netns
%endif

@Conan-Kudo
Copy link

I'd flip it to the following:

%if 0%{?rhel} && 0%{?rhel} < 8
Requires: container-selinux
Requires: slirp4netns
%else
Recommends: container-selinux
Recommends: slirp4netns
%endif

@rhatdan
Copy link
Member

rhatdan commented Aug 15, 2018

I agree, the rhel will handle centos as well.

Fixes: containers#1234
Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg vrothberg force-pushed the recommend-slirp4netns branch from 6b493ee to 7855d0a Compare August 16, 2018 06:05
@vrothberg
Copy link
Member Author

Updated, thanks!

@giuseppe
Copy link
Member

bot, retest this please

@rhatdan
Copy link
Member

rhatdan commented Aug 16, 2018

@rh-atomic-bot r+

@rhatdan
Copy link
Member

rhatdan commented Aug 16, 2018

@rh-atomic-bot retry

@rhatdan
Copy link
Member

rhatdan commented Aug 16, 2018

I am just going to merge since this passes all tests and Humu seems to not like it.

@rhatdan rhatdan merged commit 1b87fbc into containers:master Aug 16, 2018
@vrothberg vrothberg deleted the recommend-slirp4netns branch January 7, 2019 09:44
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants