[Bug 226457] Merge Review: system-config-httpd

[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=226457

--- Comment #2 from Phil Knirsch <pknirsch at redhat.com> 2010-07-23 12:46:54 EDT ---
Hi Parag.

Thanks for doing the review. Here my comments/suggestions on your initial
review.

(In reply to comment #1)
> 1) rpmlint reported
> 
> system-config-httpd.src:16: W: unversioned-explicit-obsoletes apacheconf
> system-config-httpd.src:17: W: unversioned-explicit-provides apacheconf
> system-config-httpd.src:18: W: unversioned-explicit-obsoletes
> redhat-config-httpd
> system-config-httpd.src:19: W: unversioned-explicit-provides
> redhat-config-httpd
> ==> These are old names now. Can these be removed? See
> http://fedoraproject.org/wiki/Upgrade_paths_%E2%80%94_renaming_or_splitting_packages#Do_I_need_to_Provide_my_old_package_names.3F
> 

Jup, just double checked, those can all be removed. Fixed in HG.

> system-config-httpd.src: W: invalid-url Source0:
> system-config-httpd-1.5.2.tar.gz
> ==> Add comment in SPEC like
> This is internally maintained project by Red Hat.
> 

I've actually fixed this so that the URL now points to the mercurial repo and
added a comment for the Source0 how you can generate your own tarball which
should match the upstream one 1:1 for a given tag:

URL: http://hg.fedoraproject.org/hg/system-config-httpd/
# No release tarballs typically available. To recreate your own tarball for a
specific release follow these
# steps:
# 1) hg clone http://hg.fedoraproject.org/hg/system-config-httpd/
# 2) cd system-config-httpd
# 3) hg tags
# 4) Select the release you want to create a tarball for, in this example
system-config-httpd-1_4_0-1
# 2) hg checkout system-config-httpd-1_4_0-1
# 3) ./autogen.sh
# 4) make dist
# Afterwards you should have a tarball called system-config-http-1.4.0.tar.gz
in the working directory
Source0: system-config-httpd-%{version}.tar.gz

Sounds good?

> system-config-httpd.noarch: E: explicit-lib-dependency libglade2
> == >ok
> 
> system-config-httpd.noarch: W: self-obsoletion apacheconf obsoletes apacheconf
> system-config-httpd.noarch: W: self-obsoletion redhat-config-httpd obsoletes
> redhat-config-httpd
> ==> Remove these from SPEC
>

Fixed with the above removal of the apacheconf and redhat-config-httpd
Provides/Obsoletes already.

> system-config-httpd.noarch: W: conffile-without-noreplace-flag
> /etc/alchemist/namespace/system-config-httpd/rpm.adl
> system-config-httpd.noarch: E: non-readable
> /etc/alchemist/namespace/system-config-httpd/rpm.adl 0600L
> system-config-httpd.noarch: E: non-readable
> /etc/alchemist/switchboard/system-config-httpd.switchboard.adl 0600L
> ==> If this is intended, can comments be added why noreplace flag and 600
> permission needed?
> 

Well, the problem is that both rpm.adl and system-config-httpd.switchboard.adl
should probably never be modified by admins as they provide the basic config of
the tool (which might change for never versions).

We can either make those "normal" files instead of config files (as they
shouldn't be modified in 99% of the cases) or i can add a comment, whatever you
prefer.

> system-config-httpd.noarch: E: non-executable-script
> /usr/share/system-config-httpd/ForgeBlackBox.py 0644L /usr/bin/python
> system-config-httpd.noarch: E: non-executable-script
> /usr/share/system-config-httpd/ApacheControl.py 0644L /usr/bin/python
> system-config-httpd.noarch: E: non-executable-script
> /usr/share/system-config-httpd/CacheBlackBox.py 0644L /usr/bin/python
> system-config-httpd.noarch: E: non-executable-script
> /usr/share/system-config-httpd/FileBlackBox.py 0644L /usr/bin/python
> system-config-httpd.noarch: E: non-executable-script
> /usr/share/system-config-httpd/PyAlchemist.py 0644L /usr/bin/python
> system-config-httpd.noarch: E: non-executable-script
> /usr/share/system-config-httpd/URLBlackBox.py 0644L /usr/bin/python
> system-config-httpd.noarch: E: non-executable-script
> /usr/share/system-config-httpd/CheckList.py 0644L /usr/bin/python
> system-config-httpd.noarch: E: non-executable-script
> /usr/share/system-config-httpd/ApacheConf.py 0644L /usr/bin/python
> system-config-httpd.noarch: E: non-executable-script
> /usr/share/system-config-httpd/Alchemist.py 0644L /usr/bin/python
> system-config-httpd.noarch: E: non-readable
> /etc/alchemist/namespace/system-config-httpd/local.adl 0600L
> system-config-httpd.noarch: E: non-executable-script
> /usr/share/system-config-httpd/ApacheGizmo.py 0644L /usr/bin/python
> ==>
> http://fedoraproject.org/wiki/Packaging_tricks#Remove_shebang_from_Python_libraries
> 

Fixed.

> system-config-httpd.noarch: W: dangerous-command-in-%pre mv
> system-config-httpd.noarch: W: dangerous-command-in-%preun rm
> ==> Is this needed here?
>

The mv is definitely not needed anymore, so removed that.

The rm commands i use to clean up leftover stuff in /var/cache and the pyc and
md5 check file i use. I can probably drop the pyc rm, but i'm not aware of how
else to fix the other leftover files other than removing them manually in one
of the uninstall sections of rpm. If there is i'll gladly use that.

> 2)timestamps should be preserved.Use INSTALL="install -p" when installing to
> preserve timestamps.

Added that to the make install part, looks like it's working (tested it in a
trial build locally here).

> 3) I will suggest this pacakge to follow current packaging guidelines and
> remove buildroot, %clean section and cleaning of build root in %install    

Alright, dropped it from the %install section and using %{buildroot} now
instead everywhere.

Thanks & regards, Phil

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