Bug 226036 - Merge Review: liboil
Merge Review: liboil
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 14:26 EST by Nobody's working on this, feel free to take it
Modified: 2009-06-10 13:35 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-06-10 13:35:55 EDT
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 14:26:07 EST
Fedora Merge Review: liboil

http://cvs.fedora.redhat.com/viewcvs/devel/liboil/
Initial Owner: besfahbo@redhat.com
Comment 1 Orcan Ogetbil 2009-01-17 13:27:02 EST
I reviewed this package. There are some issues, questions and suggestions that I'll bring into your attention.

* The latest version is not packaged! Please update to the latest version.

* The HACKING file should be packaged, possibly in the %doc of devel

? The examples directory can go into the %doc of devel too.

? Some files in m4 and the file "missing" indicates  GPLv2+ license.But I don't think these make their way into the package, so I guess BSD is good enough.

* I don't think glib2-devel is required to build the package. I built an identical package in mock without BR'in glib2-devel. Some of the examples require glib2-devel. So if you are going to package those examples you'll need to require glib2-devel in the devel subpackage.

* A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. The directory %{_datadir}/gtk-doc/html/ is created but not owned. So the devel package should require "gtk-doc" which is the rightful owner of that directory.

! Try to make use of the %{name} macro.

* From the SPEC file:
   # multi-jobbed make makes the build fail:
   # ./build_prototypes_doc >liboilfuncs-doc.h
   # /bin/sh: ./build_prototypes_doc: No such file or directory
   make %{?_smp_mflags}
Isn't there a contradiction here? What is that commented-out section for?

* From the SPEC file:
   # Disable Altivec, so that liboil doesn't SIGILL on non-Altivec PPCs
   # See https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=252179#c15
   #sed -i 's/CFLAGS="$CFLAGS "-maltivec""/CFLAGS="$CFLAGS "-fno-tree-vectorize -Wa,-maltivec""/' configure
   #sed -i 's/LIBOIL_CFLAGS -maltivec/LIBOIL_CFLAGS -fno-tree-vectorize -Wa,-maltivec/' configure
Do we still need these lines in the SPEC file?



Adding Colin and Caolan to CC since they were the last two known maintainers. Sorry if this was not desired
Comment 2 Orcan Ogetbil 2009-04-04 01:44:09 EDT
ping?
Comment 3 Orcan Ogetbil 2009-06-09 15:05:03 EDT
re-ping? I added more previous maintainers to the CC. Please DO NOT IGNORE!
Comment 4 Bastien Nocera 2009-06-10 10:46:09 EDT
(In reply to comment #1)
> I reviewed this package. There are some issues, questions and suggestions that
> I'll bring into your attention.
> 
> * The latest version is not packaged! Please update to the latest version.

Latest version is in devel

> * The HACKING file should be packaged, possibly in the %doc of devel

Done

> ? The examples directory can go into the %doc of devel too.

The code in examples are too complicated to be used stand-alone.

> ? Some files in m4 and the file "missing" indicates  GPLv2+ license.But I don't
> think these make their way into the package, so I guess BSD is good enough.

OK

> * I don't think glib2-devel is required to build the package. I built an
> identical package in mock without BR'in glib2-devel. Some of the examples
> require glib2-devel. So if you are going to package those examples you'll need
> to require glib2-devel in the devel subpackage.

It's optional, but used. From configure.ac:
PKG_CHECK_MODULES(GLIB, glib-2.0, HAVE_GLIB=yes, HAVE_GLIB=no)
AC_SUBST(GLIB_LIBS)
AC_SUBST(GLIB_CFLAGS)
AC_ARG_ENABLE(glib,
AC_HELP_STRING([--disable-glib],[disable usage of glib]),
[case "${enableval}" in
  yes) HAVE_GLIB=yes ;;
  no) HAVE_GLIB=no ;;
  *) AC_MSG_ERROR(bad value ${enableval} for --disable-glib) ;;
esac])
AM_CONDITIONAL(HAVE_GLIB, test "x$HAVE_GLIB" = "xyes")

> * A package must own all directories that it creates. If it does not create a
> directory that it uses, then it should require a package which does create that
> directory. The directory %{_datadir}/gtk-doc/html/ is created but not owned. So
> the devel package should require "gtk-doc" which is the rightful owner of that
> directory.

Done

> ! Try to make use of the %{name} macro.

Done in the few places where it makes sense.

> * From the SPEC file:
>    # multi-jobbed make makes the build fail:
>    # ./build_prototypes_doc >liboilfuncs-doc.h
>    # /bin/sh: ./build_prototypes_doc: No such file or directory
>    make %{?_smp_mflags}
> Isn't there a contradiction here? What is that commented-out section for?

I removed the comment.

> * From the SPEC file:
>    # Disable Altivec, so that liboil doesn't SIGILL on non-Altivec PPCs
>    # See https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=252179#c15
>    #sed -i 's/CFLAGS="$CFLAGS "-maltivec""/CFLAGS="$CFLAGS "-fno-tree-vectorize
> -Wa,-maltivec""/' configure
>    #sed -i 's/LIBOIL_CFLAGS -maltivec/LIBOIL_CFLAGS -fno-tree-vectorize
> -Wa,-maltivec/' configure
> Do we still need these lines in the SPEC file?

Nope, removed.

Should all be fixed in 0.3.16-2
Comment 5 Orcan Ogetbil 2009-06-10 13:08:35 EDT
Thank you Bastien,
There is this minor issue that is unfortunately a blocker:

The "%doc HACKING" line must come after "defattr(-,root,root,-)"

Please fix this so we can close the bug.

Thanks again!
Comment 6 Bastien Nocera 2009-06-10 13:20:42 EDT
Done in -3
Comment 7 Orcan Ogetbil 2009-06-10 13:35:55 EDT
All issues have been addressed. Thanks for the updates.

----------------------------------------------
This merge review (liboil) is APPROVED by oget
----------------------------------------------

Closing.

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