FE-NEEDSPONSOR since this is my first Fedora package.

So, I can sponsor Milind.

So reading the spec files quickly, there is a few things to clean:

1) There is no more need for BuildRoot, that's done automatically:

BuildRoot:      %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

2) The same goes for defattr()

3) Requires:	selinux-policy >= %{selinux_policyver} seems curious, where does POLICY_VERSION come from ?

4) that's minor, but I think SELinux should have the right case in the summary

5) the %files install things in  %{_datadir}/selinux/devel/, but there is no requires on the packages that own that directory (selinux-policy-devel), that seems incorrect (and I found that container-selinux has the same problem).

6) for consistency, I would reuse the macro %modulename in 
%attr(0644,root,root) %{_datadir}/selinux/devel/include/contrib/glusterd.if

7) I am sure we need to have the file in /var/lib/selinux/ tagged with %ghost, especially since that seems to be a internal directory of selinux, so that may change in the future.

(In reply to Michael Scherer from comment #2)
> So, I can sponsor Milind.
> 
> So reading the spec files quickly, there is a few things to clean:
> 
> 1) There is no more need for BuildRoot, that's done automatically:
> 
> BuildRoot:      %(mktemp -ud
> %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

Removed BuildRoot

> 
> 2) The same goes for defattr()

Removed defattr()

> 
> 3) Requires:	selinux-policy >= %{selinux_policyver} seems curious, where
> does POLICY_VERSION come from ?

Being a novice with SELinux, I'm unaware of where POLICY_VERSION comes from as well.

> 
> 4) that's minor, but I think SELinux should have the right case in the
> summary

Corrected case for SELinux in Summary

> 
> 5) the %files install things in  %{_datadir}/selinux/devel/, but there is no
> requires on the packages that own that directory (selinux-policy-devel),
> that seems incorrect (and I found that container-selinux has the same
> problem).

Does that mean selinux-policy-devel be present on production systems as well ?
I'm just going by the -devel tag now that you mentioned it.

> 
> 6) for consistency, I would reuse the macro %modulename in 
> %attr(0644,root,root) %{_datadir}/selinux/devel/include/contrib/glusterd.if

Replaced glusterd with %{modulename}

> 
> 7) I am sure we need to have the file in /var/lib/selinux/ tagged with
> %ghost, especially since that seems to be a internal directory of selinux,
> so that may change in the future.

I presume item #7 is just a comment and is not an action item on me.

Thanks Michael for the review.

So the problem is I do not know how the selinux subpackages things are supposed to be, and if there is a policy.

And I would need a link to the updated location to look at the spec file and make a more complete test with Fedora review :)

And for the -devel requires, i would suggest a sub package -devel in that package. That's the .if file, that's just needed to build a custom module (afaik), and I do not know how often this happen in practice, and if that's going to cause later some issues.

I think maybe we need to see with the fedora-selinux folks: https://github.com/orgs/fedora-selinux/people

(In reply to M. Scherer from comment #4)
> So the problem is I do not know how the selinux subpackages things are
> supposed to be, and if there is a policy.
> 
> And I would need a link to the updated location to look at the spec file and
> make a more complete test with Fedora review :)

Here's the updated spec file:
https://copr-be.cloud.fedoraproject.org/results/mchangir/glusterfs-selinux/fedora-28-x86_64/00830552-glusterfs-selinux/glusterfs-selinux.spec

I'm going to ask the (probably dumb) question here... Why isn't this just part of the main glusterfs source tree and just a subpackage in glusterfs package?

(In reply to Neal Gompa from comment #7)
> I'm going to ask the (probably dumb) question here... Why isn't this just
> part of the main glusterfs source tree and just a subpackage in glusterfs
> package?

It may happen that SELinux issues are reported independently of glusterfs. A separate RPM provides facility to fix AVC denials and other SELinux issues without the need for rebuilding and redistributing the entire glusterfs RPM set.

Milind, 

SPEC file looks fine, however I'm not able to find some scratch builds to test it, could you please send me some link?

Thanks,
Lukas.

Lukas,
FYI: https://copr.fedorainfracloud.org/coprs/mchangir/glusterfs-selinux/builds/

I hope you can access this page.

Hi, we made some refinements to the recommendations for shipping custom policy (based on feedback and bugs we encountered). Especially the spec file was significantly improved.
Please have a look and update your package accordingly: 
https://fedoraproject.org/wiki/PackagingDrafts/SELinux_Independent_Policy

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.