Bug 724942 - Review Request: libmodbus - A Modbus library written in C
Summary: Review Request: libmodbus - A Modbus library written in C
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Peter Lemenkov
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-07-22 11:45 UTC by Stéphane Raimbault
Modified: 2013-12-16 13:00 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-05-28 05:35:43 UTC
Type: ---
lemenkov: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Stéphane Raimbault 2011-07-22 11:45:09 UTC
Spec URL: http://copyleft.free.fr/data/libmodbus.spec
SRPM URL: http://copyleft.free.fr/data/libmodbus-3.0.1-1.src.rpm
Description: 
libmodbus is a free software library to send/receive data with a
device which respects the Modbus protocol. This library can use a
serial port or an Ethernet connection.

The functions included in the library have been derived from the
Modicon Modbus Protocol Reference Guide which can be obtained from
Schneider at www.schneiderautomation.com.

The license of libmodbus is LGPL v2.1 or later and the licence of programs in
tests directory is GPL v3.

The documentation is available under the Creative Commons Attribution-ShareAlike
License 3.0 (Unported) (http://creativecommons.org/licenses/by-sa/3.0/).

The official website is http://www.libmodbus.org.

The library is written in C and designed to run on Linux, Mac OS X, FreeBSD and
QNX and Windows.

Comment 1 Stéphane Raimbault 2011-07-22 11:50:55 UTC
It's my first package and I'm seeking a sponsor. I'm the maintainer of libmodbus project.

Comment 2 Peter Lemenkov 2011-07-22 12:22:29 UTC
I'll review it (and will sponsor you)

Comment 3 Peter Lemenkov 2011-07-22 12:44:31 UTC
Few notes:

* Packager field should be removed as per Fedora Policy.
* This line should be removed - "Provides: libmodbus=%{version}"
* Missing BuildRequires: xmlto, asciidoc
* Please, remove [ "%{buildroot}" != "/" ] && %{__rm} -rf %{buildroot} .Ether drop them entirely (it added by rpmbuild automatically nowadays) or simplify these lines up to "rm -rf %{buildroot}".
* Move make check into its own section from %install

%check
make check

* Don't use %makeinstall macro. Use make install DESTDIR=$RPM_BUILD_ROOT instead
* Don't mix make and %{_make} - it's not an issue, it just looks ugly.
* One more proposal to %post and %postup sections. You should simplify them as following:

%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig

This will save you two additional bash invocations during build.

* *.la files cannot be packaged at all. Remove it at the end of the %install section.

Comment 4 Veeti Paananen 2011-07-22 17:25:01 UTC
Some additional comments:

- The common format for the release tag is 1%{?dist}

- The following build requirements are part of the default build environment, so you should drop them: gcc, libtool & automake

- The devel package needs to use the following requirement to require the base package (http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package):

Requires: %{name}%{?_isa} = %{version}-%{release}

- You can avoid listing all the files explicitly by using wildcards. For example, in the base package's files section:

%{_libdir}/libmodbus.so.*

Something like this for manual pages:

%{_mandir}/man3/*.3.*

(Since the compression format may change from gzip, it's better to not to list "gz".)

And you can just list the include directory:

%{_includedir}/modbus/

> * Don't use %makeinstall macro. Use make install DESTDIR=$RPM_BUILD_ROOT
instead

Since you're already using %{buildroot} elsewhere, use make install DESTDIR=%{buildroot} since variable & macro styles may not be mixed. (http://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS)

Comment 5 Stéphane Raimbault 2011-07-22 21:52:58 UTC
(In reply to comment #3)
> %check
> make check

This target raises error so I've removed it

> * *.la files cannot be packaged at all. Remove it at the end of the %install
> section.

I've only added this files to avoid this error:

error: Installed (but unpackaged) file(s) found:
   /usr/lib64/libmodbus.la


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/lib64/libmodbus.la

Must I change the autotools rules to avoid to generate it (I don't know how and if it's possible)?

Comment 6 Stéphane Raimbault 2011-07-22 22:07:36 UTC
(In reply to comment #4)
> - The following build requirements are part of the default build environment,
> so you should drop them: gcc, libtool & automake

and autoconf too?

> - You can avoid listing all the files explicitly by using wildcards. For
> example, in the base package's files section:
> 
> %{_libdir}/libmodbus.so.*
> 
> Something like this for manual pages:
> 
> %{_mandir}/man3/*.3.*
> 
> (Since the compression format may change from gzip, it's better to not to list
> "gz".)

Great, major improvement!

What's the rule to update the changelog entry? Do I need to update it at each new release of libmobus or just when the spec file is modified?

Comment 7 Stéphane Raimbault 2011-07-22 22:22:48 UTC
Updated links and files:

Spec URL: http://copyleft.free.fr/data/libmodbus.spec
SRPM URL: http://copyleft.free.fr/data/libmodbus-3.0.1-1.f15.src.rpm

Comment 8 Veeti Paananen 2011-07-26 15:12:22 UTC
> and autoconf too?

autoconf doesn't seem to be a part of the default requirements, so keep that.

> Must I change the autotools rules to avoid to generate it (I don't know how and
if it's possible)?

As Peter already mentioned, unwanted files can removed in the %install section after make install. So in your case, add something like this there:

rm -f %{buildroot}/%{_libdir}/*.la

> What's the rule to update the changelog entry? Do I need to update it at each
new release of libmobus or just when the spec file is modified?

Add a new changelog entry every time you upload a revised spec file to your review. Make sure to increment the release number too (and note that the SRPM file name will change once you do).

Comment 9 Stéphane Raimbault 2011-07-27 18:03:00 UTC
Thank you, now it works fine so I've updated the links:
Spec URL: http://copyleft.free.fr/data/libmodbus.spec
SRPM URL: http://copyleft.free.fr/data/libmodbus-3.0.1-1.f15.src.rpm

and now?

Comment 10 Stéphane Raimbault 2011-08-01 12:17:06 UTC
lemenkov: The flag of fedora‑review still '?', could you check my changes and update the flag, please?

Comment 11 Peter Lemenkov 2011-08-01 12:41:03 UTC
REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

+/- rpmlint is not fully silent

sulaco ~/rpmbuild/SPECS: rpmlint ../RPMS/ppc/libmodbus-*
libmodbus.ppc: W: incoherent-version-in-changelog 3.0.2-1 ['3.0.1-1.fc16', '3.0.1-1']

^^^ should be 3.0.1-1 

libmodbus.ppc: W: invalid-license LGPLv2.1+
libmodbus-debuginfo.ppc: W: invalid-license LGPLv2.1+
libmodbus-devel.ppc: W: invalid-license LGPLv2.1+

^^^ Should be simply LGPLv2+

3 packages and 0 specfiles checked; 0 errors, 4 warnings.
sulaco ~/rpmbuild/SPECS:

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.

- The License field in the package spec file matches the actual license. See note regarding "License" tag above.

+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.

- The sources used to build the package, must match the upstream source, as provided in the spec URL.

sulaco ~/rpmbuild/SOURCES: sha256sum libmodbus-3.0.1.tar.gz*
37288f50e2087d73d8d43d0902fcc095ed4985304c09ec8ee3b6472e138b77d4  libmodbus-3.0.1.tar.gz
c45bd1d64a3a8970fbbfa1f6671d3f67bced9ff27b47360724aebc5512b0e0af  libmodbus-3.0.1.tar.gz.1
sulaco ~/rpmbuild/SOURCES: 

- The package doesn't successfully compile and build into binary rpms on at least one primary architecture:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3243189
http://koji.fedoraproject.org/koji/taskinfo?taskID=3243183

Please, add missing BuildRequires: automake

0 No need to handle locales.
+ The package stores shared library files in some of the dynamic linker's default paths, and it calls ldconfig in %post and %postun.
+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
0 The package DOESN'T have a %clean section (it has an empty %clean section), so it won't build cleanly on systems with old rpm (EL-4 and EL-5, not sure about EL-6). Beware.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
+ Header files are stored in a -devel package.
0 No static libraries.
+ The pkgconfig(.pc) files are stored in a -devel package and necessary runtime requirement added.
+ The library file(s) that end in .so (without suffix) is(are) stored in a -devel package.
+ The -devel package requires the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ All filenames in rpm packages are valid UTF-8.

Almost finished.
Please, 

a) Add missing BuildRequires: automake
b) Ensure that anyone could download exact the same tarball as you're using when building the package.
c) Fix "License" field
d) Fix topmost %changelog entry to match current Version and Release fields.

and tell me your FAS name to grant you packager's privileges.

Comment 12 Stéphane Raimbault 2011-08-01 20:55:31 UTC
a) I've readded automake
b) yes the tarball URL is valid
c) License fixed
d) After half an hour of battle with rpmlint about this error :

libmodbus.x86_64: W: incoherent-version-in-changelog 3.0.1-2 ['3.0.1-2.f15', '3.0.1-2.f15']

I've removed the following lines in my ~/.rpmmacros file:

%dist .f15
%fedora 15

so now, the changelog is valid and I don't need to add a '.f15' in the changelog! I'm bit puzzled because I found this recommendation in a RPM tutorial and f15 seems the right name (not fc15).

I've also removed the BuildRoot line and updated the package descriptions.

Spec URL: http://copyleft.free.fr/data/libmodbus.spec
SRPM URL: http://copyleft.free.fr/data/libmodbus-3.0.1-2.f15.src.rpm

My FAS username is stephaner

Comment 13 Stéphane Raimbault 2011-08-01 20:57:06 UTC
Fixed SRPM URL: http://copyleft.free.fr/data/libmodbus-3.0.1-2.fc15.src.rpm

Comment 14 Veeti Paananen 2011-08-01 21:08:16 UTC
Sorry about the automake confusion, I must have screwed up while testing the build requirements.

Whatever tutorial you've been following, it's wrong. You don't need to add anything to ~/.rpmmacros or any other file. For reference, these seem to be the default values for RPM in F15 (/etc/rpm/macros.dist):

%fedora 15
%dist .fc15

If/since you're not packaging for EPEL 5, the explicit pkgconfig requirement on the devel package is unnecessary. Same goes for the empty %clean section.

Comment 15 Peter Lemenkov 2011-08-02 04:55:43 UTC
Unblocking FE-NEEDSPONSOR - I just sponsored Stéphane.

Comment 16 Peter Lemenkov 2011-08-02 05:05:22 UTC
Ok, Stéphane - all my remarks were addressed by you except the one with mismatched tarball checksums.

I downloaded it again (according to the Source0 URL) and my tarball has c45bd1d64a3a8970fbbfa1f6671d3f67bced9ff27b47360724aebc5512b0e0af signature, while one you're providing has 4f193ebab5d30606e7fca3c321a9bd2f0d81598364a2d78059fca3a938906aa0.

I extracted both and checked with diff - they does differ (not only with timestamps, file owners, etc).

Please, use tarball which anyone could download instead of your own one.

Comment 17 Stéphane Raimbault 2011-08-02 05:40:46 UTC
OK, I didn't know it was necessary to use the tarball indicated in the spec file URL so I've built again the package with the 3.0.1 tarball of github (curl -LRO)

I've also removed the pkg-config requirement and the clean target.

Spec URL: http://copyleft.free.fr/data/libmodbus.spec
SRPM URL: http://copyleft.free.fr/data/libmodbus-3.0.1-2.fc15.src.rpm

Comment 18 Peter Lemenkov 2011-08-02 05:51:37 UTC
(In reply to comment #17)
> OK, I didn't know it was necessary to use the tarball indicated in the spec
> file URL so I've built again the package with the 3.0.1 tarball of github (curl
> -LRO)
> 
> I've also removed the pkg-config requirement and the clean target.
> 
> Spec URL: http://copyleft.free.fr/data/libmodbus.spec
> SRPM URL: http://copyleft.free.fr/data/libmodbus-3.0.1-2.fc15.src.rpm

Good. This package is


APPROVED.

Comment 19 Peter Lemenkov 2011-08-03 09:55:11 UTC
Ping, Stéphane! :)
Please, proceed further as described here - we're all waiting for you now :)

https://fedoraproject.org/wiki/Package_SCM_admin_requests

Comment 20 Stéphane Raimbault 2011-08-03 11:09:39 UTC
Let's go!

New Package SCM Request
=======================
Package Name: libmodbus
Short Description: A Modbus library
Owners: stephaner
Branches: f15
InitialCC: 

Peter, feel free to add your FAS name in InitialCC.

Comment 21 Gwyn Ciesla 2011-08-03 11:57:33 UTC
Git done (by process-git-requests).

Added f16 branch.

Comment 22 Stéphane Raimbault 2011-08-07 14:31:16 UTC
I've tried to build my package for f17 but the build fails with this error:

+ autoreconf
src/Makefile.am:1: Libtool library used but `LIBTOOL' is undefined
src/Makefile.am:1:   The usual way to define `LIBTOOL' is to add `AC_PROG_LIBTOOL'
src/Makefile.am:1:   to `configure.ac' and run `aclocal' and `autoconf' again.
src/Makefile.am:1:   If `AC_PROG_LIBTOOL' is in `configure.ac', make sure
src/Makefile.am:1:   its definition is in aclocal's search path.
autoreconf: automake failed with exit status: 1

I don't use AC_PROG_LIBTOOL in my configure.ac because these options are deprecated http://www.gnu.org/software/libtool/manual/html_node/LT_005fINIT.html

Do I need to check against the version of libtool LT_PREREQ([2.4]) or to use older options for backward compatibility?

Comment 23 Stéphane Raimbault 2011-08-11 22:02:47 UTC
I've fixed the problem by adding libtool to BuildRequires certainly because automake and libtool need to be at the same location (or something like that) (so Veeti it wasn't a good idea to remove them :(

Is it possible to include my package for fedora 16?

Comment 24 Peter Lemenkov 2011-08-12 04:12:10 UTC
Hello, Stéphane.
Of course, it's possible. You need to 

a) Clone git repository already created (fedpkg clone libmodbus)
b) Ensure that your SRPM can be built in Koji (koji build --scratch dist-rawhide /path/to/your/libmodbus.src.rpm
c) Import your package into Git (fedpkg import /path/to/your/libmodbus.src.rpm)
d) Commit and push your changes (git commit -" some descriptive message" && git push"
d) Build package (fedpkg build)
e) Use Bodhi magic to request inclusion into updates-testing ( https://admin.fedoraproject.org/updates/new/ )
f) After a week (of testing (or after a several positive feedbacks) you should request inclusion into  updates ( https://admin.fedoraproject.org/updates/new/ )

That's it. Don't hesitate to contact me directly if you have any questions.

Comment 25 Peter Lemenkov 2011-12-17 16:04:19 UTC
No packages for F-15 and F-16 yet. Stéphane, is it intentional?

Comment 26 Peter Lemenkov 2012-05-28 05:35:43 UTC
Since F-17 (where this package is available) will be released very soon and F-15 will be EOLed I believe that builds for F-15 and F-16 are not that interesting. So I'm closing this.

Comment 27 John Morris 2013-12-15 19:36:10 UTC
Package Change Request
======================
Package Name: libmodbus
New Branches: el6
Owners: zultron
InitialCC: 

Requesting new branch for el6.  Fedora package owner expressed agreement in #1039212.

Comment 28 Gwyn Ciesla 2013-12-16 13:00:24 UTC
Git done (by process-git-requests).


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