Further information:

Following the Packaging guidlines, I also have build on koji and it builds fine for F12 and F13 in koji:
http://koji.fedoraproject.org/koji/tasks?state=closed&owner=manpaz

Thanks.

> Summary:        A userspace USB HID access library

It's wide-spread taste practise to omit leading articles, such as "A" or "The":
Summary: Userspace USB HID access library


> License:        GPLv3+

Doesn't seem to be true.

[qa@faldor libhid-0.2.17]$ grep -i "version 3" * -R|wc -l
0
[qa@faldor libhid-0.2.17]$ grep -i "version 2" * -R|wc -l
22


* %description of -devel packages mentions "static" libraries, which are not included.


> # Force use of system libtool

The big question here is: Why?


> %makeinstall

https://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used


> %defattr(0644,root,root)
> %{_mandir}/man1/*

The extra %defattr here is acceptable, but dangerous. It wouldn't be the first time somebody adds files somewhere below and overlooks the 0644 mode. If you need to fix the man page permissions, chmod after %setup is safer.


* libhid-devel is missing "Requires: libusb-devel", since it depends on usb.h and libhid.pc needs -lusb.

Thank your for your advise. Here the changes I've made:

1. I removed the "A" article.

2. The GPL Version 3 comes from automake at build time, but looking at the upstream website http://libhid.alioth.debian.org/, they claim libhid is released under the terms of the GPLVersion 2, so I've fixed it to GPLv2

3. The upstream says -devel includes static libraries,  but the Package Gideline says to put static on a -static package, and take this action only if static is really necessary, so I'm removed static libraries, no I'm removing "static library" statement from -devel description.

4. I'm removing --force from libtoolize, I just copied the whole command with parameters as "autogen.sh" does, but now I'm using "make DESTDIR=$RPM_BUILD_ROOT install" as suggested in the link provided by you.

5. %deffattr(0644,root,root) I left because I was getting trouble with manpages, but then I create a patch and forgot to remove %deffattr, sorry, my bad.

6. libusb-devel added to libhid-devel.

I rebuild release 2 on Koji and the build was completed successfully.
http://koji.fedoraproject.org/koji/tasks?state=closed&owner=manpaz

Once again, thank you for your advise.

BTW, I forgot to put the links for the new SRPM with the proper release number.

Spec URL: http://repo.bashlinux.com/rpm/fedora/12/bashlinux/SPECS/libhid.spec
SRPM URL: http://repo.bashlinux.com/rpm/fedora/12/bashlinux/SRPMS/libhid-0.2.17-2.fc12.src.rpm

Thank you,

A closer look at the package:


> 2.

I still don't understand why it was "GPLv3+", as the license of any autoconf/automake or libtool files typically is not included in the rpm's "License" tag.


> 4.

The comment "Force use of system libtool" is misleading. Actually, you don't want to enforce anything. You just want to build an initial autotools/libtool framework, because the svn snapshot doesn't include those files. Running the included autogen.sh script would work, but it would also execute ./configure directly.

Copying from autogen.sh works, but should be done in %prep section after %setup, as it is a form of source preparation to be executed once only (just like applying patches). Doing it %prep would also be beneficial for --short-circuit builds.

And instead of running all the tools manually, you could run just "autoreconf -i".


> --enable-maintainer-mode

Why do you enable it? (especially for this svn snapshot where you regenerate the files already)


> rm -rf $RPM_BUILD_ROOT%{_libdir}/*.la

The .la file in  %{python_sitearch}/*/*.la  should be deleted, too. Btw, it's broken anyway if you delete the other one in %_libdir.


> sed -ie /pkgpy/s/libhid/hid/g Makefile          # Use 'hid' module name instead 'libhid'
> sed -ie /pkgpy/s/libhid/hid/g swig/Makefile     # Use 'hid' module name instead 'libhid'

The comment is insufficient. *Why* is this done? And it substitutes more than intended in swig/Makefile. Take a look at the diff against the original file.

Both modifications ought to be applied as %patch files instead, which is less fragile. These two sed substitutions would fail silently, and your %files section is not explicit enough due to wildcards and would include _any_ Python module and not just 'hid'.


* The diff against -1.fc12:

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

Why that change? The new one doesn't match the guidelines:
https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag


> -make CFLAGS="$CFLAGS"
> -
> +make CFLAGS="$CFLAGS" 

The extra CFLAGS definition here should not be necessary and would be the wrong place where to do it. As can be seen in "rpm --eval %configure", CFLAGS are defined and exported already when running the "configure "script. [If you needed to "fix" the CFLAGS usage, the proper place would be to fix the configure script and/or the Makefiles.]

"make" should also be run as "make %{?_smp_mflags}":
https://fedoraproject.org/wiki/Packaging:Guidelines#Parallel_make


> %defattr(-,root,root)

https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions


> +%ifnarch i686
>  %{python_sitearch}/*
> +%endif

I guess this is because rpmbuild warns about duplicate file entries on platforms (not limited to i686) where %{python_sitearch} and %{python_sitelib} are the same.

But actually this is completely harmless _as long as_ you include the Python files in the same subpackage. And rpmbuild then does not include any file more than once.

https://fedoraproject.org/wiki/Packaging:Guidelines#Duplicate_Files
is merely important for packages with multiple subpackages and multiple %files sections, where files may be included in _more than one_ subpackage by accident.

If you have strong feelings about it, you could do

  %{python_sitelib}/*
  %ifarch x86_64 ppc64 sparc64 s390x
  %{python_sitearch}/*
  %endif

to cover more platforms correctly, but it doesn't add much value. A brief comment instead would be fine.

Thanks for setting up this SRPM.  I used it on a Fedora 12 i686 system to build (successfully) the Measurement Computing drivers available from 

  ftp://lx10.tx.ncsu.edu/pub/Linux/drivers/USB/

I had to remove -static from the Makefile.  Should there be a static libhid.a library built along side the dynamic one?

Your very welcome John,

I don't think a -static subpackage is being necessary here.
http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries

But I don't have the last word.

Michael, can you please confirm that package is not necessary please?

Thanks.

A handcrafted Makefile that links all executables (even test programs) statically is not reason enough to build the static lib and create a -static subpackage.

[Btw, in the Fedora package collection, linking statically with a library, which is available also as a shared lib, would require explicit approval from FESCo plus usage of the -static sub-package in the build dependencies.]

Thanks Michael, great advise, I appreciate your hard work.

Well, answering in the same way:

> 2.
I set "GPLv3+" manually because I saw on COPYING file, which originally doesn't come from SVN, but it is there after run autogen.sh, then I realise the upstrem claims in its website that libhid is released under "GPLv2".

> 4. 
Thanks, "autoreconf -i" does the work really nice.

> --enable-maintainer-mode
Sorry, I left this option there when I was trying to fix an error with manpages generation, then I forgot to remove.

> rm -rf ${RPM_BUILD_ROOT%{_libdir}/*.la
Removed.

> sed -ie /pkgpy/s/libhid/hid/g Makefile          # Use 'hid' module name instead 'libhid'
> sed -ie /pkgpy/s/libhid/hid/g swig/Makefile     # Use 'hid' module name instead 'libhid'
I had to have this becuase, previous releases were using hid as namespace for python module, and my apps was being broken.  Well, I should admit the package is not only for my use, so I remove the lines from spec file and fixed my apps, Sorry, my bad.

> -BuildRoot:     %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
> +BuildRoot:     %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXX)
Typo error from me, I just tried root instead XXXXXX, then restored back the wrong string. Sorry again.

> -make CFLAGS="$CFLAGS"
> -
> +make CFLAGS="$CFLAGS" 
CFLAGS is no longer necessary because "autoreconf" is doing very well the work, so I'm removing CFLAGS and adding %{?_smp_mflags}.

> %defattr(-,root,root)
File permissions now has been set to %defattr(-,root,root,-) as defined in the guidelines


> +%ifnarch i686
>  %{python_sitearch}/*
> +%endif

Fixed conditional as suggested:
  %{python_sitelib}/*
  %ifarch x86_64 ppc64 sparc64
  %{python_sitearch}/*
  %endif

Please, note the s390x architecture is not included, since upstream doesn't recommend to build libhid on such arch.

At the end, I'd like to ask you, if you mind to check if the patch comment is ok. I'm applying this patch since the upstream is building manpages with xsltproc with the xslt templates in a location that only works on Debian, so I'm using db2x_docbook2man in order to generate manpages safely. This is why I have added the brief comment, as the Guidelines suggest.
https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

New release is available at:
Spec URL: http://repo.bashlinux.com/rpm/fedora/12/bashlinux/SPECS/libhid.spec
SRPM URL:
http://repo.bashlinux.com/rpm/fedora/12/bashlinux/SRPMS/libhid-0.2.17-3.fc12.src.rpm

Thank you for your time and patience, I'll be waiting for new comments. Whenever you have a chance.

(In reply to comment #8)
> A handcrafted Makefile that links all executables (even test programs)
> statically is not reason enough to build the static lib and create a -static
> subpackage.

I agree, now that I understand the static lib policy.  I was just assuming the person who wrote the Makefile for this device driver had some reason for compiling with -static.  Maybe that choice was driven by something that isn't present on Fedora systems.

It appears to be personal preference only. A single file for each program, and no dependency on any external DSOs. Some people like to do that even if it isn't necessary.


Sometimes but rarely there is a reason for static linking. E.g. just recently a tiny plugin interface library in the Package Review queue required static linking. Its implementation used C/C++ static storage, which would be shared by multiple plugins dlopen'ed by the same process. Only by linking the library statically with every plugin, each plugin gets its own independent storage.

* Static Linking Considered Harmful
http://people.redhat.com/drepper/no_static_linking.html

* There is a small camp of people which like to link _special_ applications statically,
https://fedoraproject.org/wiki/User:Pertusus#On_static_library_packaging_for_numerical_and_data_processing
However, that doesn't have backup from Fedora's packaging comittee.

Re: comment 9

> db2x_docbook2man

Without the patch, it creates a bad man page here. It executes:

xsltproc -''-nonet /usr/share/sgml/docbook/xsl-stylesheets-1.75.2/manpages/docbook.xsl libhid-detach-device.dbk

which _is_ a valid path to the stylesheet file:

$ rpm -qf /usr/share/sgml/docbook/xsl-stylesheets-1.75.2/manpages/docbook.xsl
docbook-style-xsl-1.75.2-4.fc12.noarch

But it prints many warnings/errors, and the created manual contains mistakes and is lacking substitutions.

So, your patch to use db2x_docbook2man creates a good man page.


> libhid-0.2.17-3.fc12.src.rpm

%patch0 must be applied before running autoreconf, or else the generated Makefile.in would be based on the unpatched Makefile.am


> -make CFLAGS="$CFLAGS" 
> +make %{?_smp_mflags}

Good. This one reveals that ./m4/md_conf_debugging.m4 messes with the compiler flags. It strips -g from the $CFLAGS by default. And one cannot simply configure with --enable-debug, because then it modifies the optimisation flags.

The ugly work-around would be to put back the CFLAGS definition when running "make". The better fix would be to patch the configure script and stop it from messing with the flags like this.


> -%ifnarch i686
> +# Platforms in which libhid must build fine
> +%ifarch x86_64 ppc64 ppc sparc64

The comment doesn't match the purpose of that %ifarch, though. This %ifarch is only for those platforms where both macros expand to a separate directory, to avoid rpmbuild's warning about "duplicate files entries" in the spec file (comment 5). On "ppc", %{python_sitearch} expands to /usr/lib just like %{python_sitelib}. Hence you get that warning for a ppc build.


Don't see any other issues with the package.

Ok, here the new changes:

> db2x_docbook2man
> libhid-0.2.17-3.fc12.src.rpm

I have added patch0 before autoreconf, in order to fix.

> -make CFLAGS="$CFLAGS" 
> +make %{?_smp_mflags}

I have added patch1 in order to path the configure script.

> -%ifnarch i686
> +# Platforms in which libhid must build fine
> +%ifarch x86_64 ppc64 ppc sparc64

Changed the comment for %ifarch and removed ppc from the conditional.

Tested on Koji and it builds fine, also rpmlint didn't show issues with -debuginfo subpackage.

New release and spec file are available at:
Spec URL: http://repo.bashlinux.com/rpm/fedora/12/bashlinux/SPECS/libhid.spec
SRPM URL: http://repo.bashlinux.com/rpm/fedora/12/bashlinux/SRPMS/libhid-0.2.17-4.fc12.src.rpm

As always, Thank you.

Okay, that did it. :)  Be painstakingly careful when modifying it for future updates or upgrades.

APPROVED

New Package CVS Request
=======================
Package Name: libhid
Short Description: A userspace USB HID acess library
Owners: manpaz
Branches: F-12, F-13
InitialCC:

CVS done (by process-cvs-requests.py).

Build is available for Rawhide (F-14).

For F-12 and F-13, the builds would need to be released through bodhi:
https://admin.fedoraproject.org/updates/

Builds fro F-12 and F-13 have been submitted to bodhi.