[Bug 508922] Review Request: system-config-selinux - GUI Code for system-config-selinux, polgen, and lockdown

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


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


David Timms <dtimms at iinet.net.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtimms at iinet.net.au
         AssignedTo|nobody at fedoraproject.org    |dtimms at iinet.net.au




--- Comment #13 from David Timms <dtimms at iinet.net.au>  2009-09-27 09:16:36 EDT ---
(In reply to comment #12)
> Can we get this approved,  I really want to get it out in Fedora 12.  
First step is reviewed / improved ...

(In reply to comment #11)
I don't think I have enough experience to be approving security related
packages in Fedora. However, some initial nits:

1. package name differs from web page, so maybe fix the web page from
(System-Config-Selinx) !

2. repeated requires:
Requires: selinux-policy
Requires: selinux-policy >= 3.6.28-4
-> which is it ?, I don't think it makes sense to have both.

3. Can you confirm that this package replaces policycoreutils-gui ?
Does the new package include all the old package's functionality ?

4. The normal place to put the python_sitelib call is at the top of the .spec,
see:
http://fedoraproject.org/wiki/Packaging:Python#System_Architecture

5. description: typo in (graphcial). Perhaps instead of just repeating short
names/acronyms, we could have something like spelling out the acronym during
first stating of it:
---
This package contains two graphical tools for adjusting the mandatory access
control security settings, as implemented by Security Enhanced Linux (SELinux).
system-config-selinux provides an interface for configuring and managing
SELinux, while selinux-polgengui is used to generate SELinux policy modules.
---
-> feel free to improve upon that !

6. For consistency, keep the two-line breaks between sections (for post, clean
files, install)

7. Nice to see lines like ln... to be split over two lines, limiting spec to be
mostly <= 80 chars wide.

8. avoid use of tab char, space is easier to peruse.

9. changelog: is not in required format:
- space between * and first char of date.
- space between - and item text
- version-release is not included for any entries. (think the space is missing
as well, making those entries look weird.
- subprocesss ! 
- lines longer than 80chars.

10. how were the labelling problems fixed (were they added to the -targeted
policy) ?

11. Is there any doc files that need to be included ?

12. Who is going to own this package going forward, since it seems the reporter
is having trouble keeping the spec file consistent with itself, let alone the
fedora packaging standards, is this a time problem ?

13. files: 
-config(noreplace) %{_datadir}/
  -> are people expected to make changes to configuration in /usr/share ?
-%{_sysconfdir}/dbus-1/system.d/org.fedoraproject.selinux.config.conf
  -> is this a file that needs the noreplace, ie are users expected to adjust
this (potentially) ?

14. gui tools require icons, are some included, do some need to be requested of
the artwork project ?

I haven't explicitly checked Provides nor BuildRequires.
Hopefully this helps move the review a little further on.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.