Bug 225798 - Merge Review: gimp-help
Merge Review: gimp-help
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 13:44 EST by Nobody's working on this, feel free to take it
Modified: 2008-12-13 14:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-12-13 14:11:01 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:44:15 EST
Fedora Merge Review: gimp-help

http://cvs.fedora.redhat.com/viewcvs/devel/gimp-help/
Initial Owner: nphilipp@redhat.com
Comment 1 Orcan Ogetbil 2008-10-30 14:48:57 EDT
I reviewed this package. The following issues need to be addressed for compilance with the guidelines:

* The files AUTHORS, ChangeLog, COPYING, HACKING, NEWS, README, TERMINOLOGY need to be listed under %doc

* The package owns
   %dir %{_datadir}/gimp
   %dir %{_datadir}/gimp/%{gimpsubver}
which are also owned by gimp. Note that the package Requires: gimp. The above ownerships need to be dropped.

* %defattr(-,root,root,-) is preferred. 

* BuildRequires: libxml2
is not required since
   BuildRequires: libxslt
will pull that up

* Similarly
   BuildRequires: docbook-style-xsl
is not required. It will be pulled up by other dependencies. Am I wrong?
Comment 2 Orcan Ogetbil 2008-12-10 14:05:55 EST
ping?
Comment 3 Nils Philippsen 2008-12-11 08:29:35 EST
Sorry for the late reply.

I've fixed the following in CVS, gimp-2.4.2-2 is building with these changes for Rawhide right now.

(In reply to comment #1)
> I reviewed this package. The following issues need to be addressed for
> compilance with the guidelines:
> 
> * The files AUTHORS, ChangeLog, COPYING, HACKING, NEWS, README, TERMINOLOGY
> need to be listed under %doc

These files get shipped except HACKING which is documentation about how to change the documentation for which you need the source anyway.

> * The package owns
>    %dir %{_datadir}/gimp
>    %dir %{_datadir}/gimp/%{gimpsubver}
> which are also owned by gimp. Note that the package Requires: gimp. The above
> ownerships need to be dropped.

Not owned anymore.

> * %defattr(-,root,root,-) is preferred. 

Fixed.

> * BuildRequires: libxml2
> is not required since
>    BuildRequires: libxslt
> will pull that up
> 
> * Similarly
>    BuildRequires: docbook-style-xsl
> is not required. It will be pulled up by other dependencies. Am I wrong?

The configure script of gimp-help checks directly for e.g. xmllint (included in libxml2) and .../xhtml/docbook.xsl which is included in docbook-style-xsl. I.e. both packages are required directly, therefore I keep listing them as build requirements. It doesn't matter that they in turn are required by other build requirements, I don't want to rely on that this fact doesn't change in the future.
Comment 4 Orcan Ogetbil 2008-12-11 15:17:18 EST
Thanks for the update. There are two more little things:

* From the SPEC file:
    cat << EOF > files.list
    %%defattr (-, root, root, -)
    %%dir %{_datadir}/gimp/%{gimpsubver}/help
    %{_datadir}/gimp/%{gimpsubver}/help/images
    EOF
and
    echo "%%lang($dir) %{_datadir}/gimp/%{gimpsubver}/help/$dir" >> "$f"

The macro entries with single % expand into the files.list . This is equivalent to putting hard-coded paths into %files . I think those macro entries need double-% too.

* Parallel make must be supported whenever possible (If it is not supported, this should be noted in the SPEC file as a comment.). Sorry that I missed this in my initial review. Enabling the parallel make didn't cause a problem with my dual core. It certainly speeds things up.
Comment 5 Nils Philippsen 2008-12-12 05:53:46 EST
(In reply to comment #4)
> Thanks for the update. There are two more little things:
> 
> * From the SPEC file:
>     cat << EOF > files.list
>     %%defattr (-, root, root, -)
>     %%dir %{_datadir}/gimp/%{gimpsubver}/help
>     %{_datadir}/gimp/%{gimpsubver}/help/images
>     EOF
> and
>     echo "%%lang($dir) %{_datadir}/gimp/%{gimpsubver}/help/$dir" >> "$f"
> 
> The macro entries with single % expand into the files.list . This is equivalent
> to putting hard-coded paths into %files . I think those macro entries need
> double-% too.

Technically they needn't be, as it doesn't really matter if they're expanded while files.list is built or while it is evaluated by rpmbuild. I've changed it to be more consistent, though.

> * Parallel make must be supported whenever possible (If it is not supported,
> this should be noted in the SPEC file as a comment.). Sorry that I missed this
> in my initial review. Enabling the parallel make didn't cause a problem with my
> dual core. It certainly speeds things up.

I've changed that and started building gimp-2.4.2-3.fc11 with these changes.
Comment 6 Orcan Ogetbil 2008-12-12 11:31:39 EST
(In reply to comment #5)
> (In reply to comment #4)
> > The macro entries with single % expand into the files.list . This is equivalent
> > to putting hard-coded paths into %files . I think those macro entries need
> > double-% too.
> 
> Technically they needn't be, as it doesn't really matter if they're expanded
> while files.list is built or while it is evaluated by rpmbuild. I've changed it
> to be more consistent, though.
> 
You're right. I thought more about it. It shouldn't matter.

> > * Parallel make must be supported whenever possible (If it is not supported,
> > this should be noted in the SPEC file as a comment.). Sorry that I missed this
> > in my initial review. Enabling the parallel make didn't cause a problem with my
> > dual core. It certainly speeds things up.
> 
> I've changed that and started building gimp-2.4.2-3.fc11 with these changes.

I checked koji. It looks all good. 

-------------------------------------------
The package "gimp-help" is APPROVED by oget
-------------------------------------------
Comment 7 Orcan Ogetbil 2008-12-13 14:11:01 EST
Package is in rawhide. We can close the bug now.

Note You need to log in before you can comment on or make changes to this bug.