Bug 990272

Only skimmed over the spec:


> Name: libmbim
> Group: Development/Libraries

The group for run-time libs has been "System Environment/Libraries" for many years. Alternatively, drop the Group tag.


> Requires: glib2 >= %{glib2_version}

Add %{?_isa} here and also a comment giving the rationale why this minimum version is strictly needed.

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


> %package devel
> Requires: glib2-devel

Add %{?_isa} here, too.


> Requires: pkgconfig

There's an automatic dep on /usr/bin/pkgconfig already.


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

Not needed, and we don't run ldconfig for -devel packages.


> %files devel
> %dir %{_includedir}/libmbim-glib
> %{_includedir}/libmbim-glib/*.h

A single line could replace those two above, too:

  %{_includedir}/libmbim-glib/

> %dir %{_datadir}/gtk-doc/html/libmbim-glib
> %{_datadir}/gtk-doc/html/libmbim-glib/*

If you don't let the package require "gtk-doc", own all directories:

  %dir %{_datadir}/gtk-doc
  %dir %{_datadir}/gtk-doc/html

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

Of course, you could include the entire tree with a single line:

  %{_datadir}/gtk-doc/

(In reply to Michael Schwendt from comment #1)
> Only skimmed over the spec:
> 
> 
> > Name: libmbim
> > Group: Development/Libraries
> 
> The group for run-time libs has been "System Environment/Libraries" for many
> years. Alternatively, drop the Group tag.

Dropped.

> 
> > Requires: glib2 >= %{glib2_version}
> 
> Add %{?_isa} here and also a comment giving the rationale why this minimum
> version is strictly needed.
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

As F18+ and later meet the minimum requirement, I've removed the explicit version requirement and added {?_isa}.

> 
> > %package devel
> > Requires: glib2-devel
> 
> Add %{?_isa} here, too.

Done.

> > Requires: pkgconfig
> 
> There's an automatic dep on /usr/bin/pkgconfig already.

Removed.

> > %post   devel -p /sbin/ldconfig
> > %postun	devel -p /sbin/ldconfig
> 
> Not needed, and we don't run ldconfig for -devel packages.

Removed.

> > %files devel
> > %dir %{_includedir}/libmbim-glib
> > %{_includedir}/libmbim-glib/*.h
> 
> A single line could replace those two above, too:
> 
>   %{_includedir}/libmbim-glib/

Done.

> > %dir %{_datadir}/gtk-doc/html/libmbim-glib
> > %{_datadir}/gtk-doc/html/libmbim-glib/*
> 
> If you don't let the package require "gtk-doc", own all directories:
> 
>   %dir %{_datadir}/gtk-doc
>   %dir %{_datadir}/gtk-doc/html

Done.

> https://fedoraproject.org/wiki/Packaging:
> Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your
> _package_to_function
> 
> Of course, you could include the entire tree with a single line:
> 
>   %{_datadir}/gtk-doc/

gli2 also provides some gtk-doc, and while that's currently stored in glib2-doc it has switched around in the past between -devel (which is a BuildRequire) and -docs so I'm more comfortable explicitly owning the relevant dirs than all of them.

Specfile and SRPM updated in same location as comment 1.  Thanks!

Why can't you bump the version? Is it too hard for you to do that?

One issue: You should avoid using %{__rm}.

(In reply to Christopher Meng from comment #3)
> Why can't you bump the version? Is it too hard for you to do that?

Bump which version?

> One issue: You should avoid using %{__rm}.

Do you mean that the specfile should use 'rm' directly instead?

"bump the version" likely refers to the "Release" tag:

https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes
| 
| Increase the "Release" tag every time you upload a new package
| to avoid confusion. The reviewer and other interested parties
| probably still have older versions of your SRPM lying around
| to check what has changed between the old and new packages;
| those get confused when the revision didn't change. 

It would be more helpful, if Christopher would give a rationale (or a link to the Wiki) when asking for spec file changes.

[...]

A close look at the package:


> %global snapshot .git20130730
> Release: 1%{snapshot}%{?dist}

If following the guidelines, the date would come before "git":
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages

That's not really important for post-release snapshots and when strictly keeping the same package versioning scheme, but it helps avoiding surprises, such as comparing letters with numbers:

  $ rpmdev-vercmp 1.git20130830.fc19 1.20130730git.fc19
  1.git20130830.fc19 < 1.20130730git.fc19


A test-build:

> configure: WARNING: unrecognized options: --with-tests

?


> checking whether to build gtk-doc documentation... no
>    Documentation:           no

?


>    Maintainer mode:         yes

Indeed, it's one of the configure scripts that still enable AM_MAINTAINER_MODE. Not an immediate problem, just pointing it out.


> checking for LIBMBIM_GLIB... no
> configure: error: Package requirements (glib-2.0 >= 2.32
>                   gobject-2.0
>                   gio-2.0
>                   gudev-1.0 >= 147) were not met:
> No package 'gudev-1.0' found

It's missing:

  # libgudev1-devel
  BuildRequires: pkgconfig(gudev-1.0) >= 147


> Requires: glib2%{?_isa}

So, if there is _no_ minimum version requirement, the conclusion is that you should drop this explicit Requires completely in accordance with:
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

There is an automatic arch-specific dependency on the libglib-2.0.so.0 SONAME.


> make %{?_smp_mflags}

Two options for making the build output more verbose (so one could actually see what compiler flags are used or examine build logs with scripts). Either:

  V=1 make %{?_smp_mflags}
or
  %configure --disable-silent-rules ...


Relevant rpmlint output for the binary rpms:

> libmbim.x86_64: E: incorrect-fsf-address /usr/share/doc/libmbim-1.5.0/COPYING

https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address


> libmbim.x86_64: E: zero-length /usr/share/doc/libmbim-1.5.0/README

Not a big issue. You could drop it, but decide yourself whether you would notice that a future upgrade would fill it with contents.


> libmbim-utils.x86_64: W: no-documentation
> libmbim-utils.x86_64: W: no-manual-page-for-binary mbimcli
> libmbim-utils.x86_64: W: no-manual-page-for-binary mbim-network

Just including these warnings to meet the review guidelines. ;)


> %package utils
> License: GPLv2+

GPLv2 and GPLv2+, since the mbim-network script contains a GPLv2 preamble with no "or later" clause.

Perhaps that's intentional, perhaps not:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Clarification


> glib2 also provides some gtk-doc, and while that's currently stored
> in glib2-doc it has switched around in the past between -devel (which
> is a BuildRequire) and -docs so I'm more comfortable explicitly
> owning the relevant dirs than all of them.

There is no reason to be concerned, because we install into an empty %{buildroot}, and that one won't include any docs from glib2 build requirements.

> %dir %{_datadir}/gtk-doc
> %dir %{_datadir}/gtk-doc/html
> %dir %{_datadir}/gtk-doc/html/libmbim-glib
> %{_datadir}/gtk-doc/html/libmbim-glib/*

  %{_datadir}/gtk-doc/

achieves exactly the same thing.

  $ rpmls -p libmbim-devel-1.5.0-1.git20130730.fc19.x86_64.rpm|grep ^d
  drwxr-xr-x  /usr/include/libmbim-glib
  drwxr-xr-x  /usr/share/gtk-doc
  drwxr-xr-x  /usr/share/gtk-doc/html
  drwxr-xr-x  /usr/share/gtk-doc/html/libmbim-glib

(In reply to Michael Schwendt from comment #5)
> "bump the version" likely refers to the "Release" tag:
> 
> https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes
> | 
> | Increase the "Release" tag every time you upload a new package
> | to avoid confusion. The reviewer and other interested parties
> | probably still have older versions of your SRPM lying around
> | to check what has changed between the old and new packages;
> | those get confused when the revision didn't change. 
> 
> It would be more helpful, if Christopher would give a rationale (or a link
> to the Wiki) when asking for spec file changes.
> 
> [...]
> 
> A close look at the package:
> 
> 
> > %global snapshot .git20130730
> > Release: 1%{snapshot}%{?dist}
> 
> If following the guidelines, the date would come before "git":
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages
> 
> That's not really important for post-release snapshots and when strictly
> keeping the same package versioning scheme, but it helps avoiding surprises,
> such as comparing letters with numbers:
> 
>   $ rpmdev-vercmp 1.git20130830.fc19 1.20130730git.fc19
>   1.git20130830.fc19 < 1.20130730git.fc19

Changed.

> A test-build:
> 
> > configure: WARNING: unrecognized options: --with-tests

Fixed.

> > checking whether to build gtk-doc documentation... no
> >    Documentation:           no

The documentation is pre-generated to ensure that multilib builds don't conflict.  So the documentation is pre-built, but won't be built during the package build.

> >    Maintainer mode:         yes
> 
> Indeed, it's one of the configure scripts that still enable
> AM_MAINTAINER_MODE. Not an immediate problem, just pointing it out.

I'll poke upstream on it.

> > checking for LIBMBIM_GLIB... no
> > configure: error: Package requirements (glib-2.0 >= 2.32
> >                   gobject-2.0
> >                   gio-2.0
> >                   gudev-1.0 >= 147) were not met:
> > No package 'gudev-1.0' found
> 
> It's missing:
> 
>   # libgudev1-devel
>   BuildRequires: pkgconfig(gudev-1.0) >= 147

Fixed.

> > Requires: glib2%{?_isa}
> 
> So, if there is _no_ minimum version requirement, the conclusion is that you
> should drop this explicit Requires completely in accordance with:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
> 
> There is an automatic arch-specific dependency on the libglib-2.0.so.0
> SONAME.

Requires removed; relying on auto deps.

> > make %{?_smp_mflags}
> 
> Two options for making the build output more verbose (so one could actually
> see what compiler flags are used or examine build logs with scripts). Either:
> 
>   V=1 make %{?_smp_mflags}
> or
>   %configure --disable-silent-rules ...

Fixed to "V=1 make"

> Relevant rpmlint output for the binary rpms:
> 
> > libmbim.x86_64: E: incorrect-fsf-address /usr/share/doc/libmbim-1.5.0/COPYING
> 
> https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address

Poked upstream, they fixed it, new tarball.

> > libmbim.x86_64: E: zero-length /usr/share/doc/libmbim-1.5.0/README
> 
> Not a big issue. You could drop it, but decide yourself whether you would
> notice that a future upgrade would fill it with contents.

Poked upstream, they fixed it, new tarball.

> > libmbim-utils.x86_64: W: no-documentation
> > libmbim-utils.x86_64: W: no-manual-page-for-binary mbimcli
> > libmbim-utils.x86_64: W: no-manual-page-for-binary mbim-network
> 
> Just including these warnings to meet the review guidelines. ;)

Upstream is discussing how to add the manpages.

> > %package utils
> > License: GPLv2+
> 
> GPLv2 and GPLv2+, since the mbim-network script contains a GPLv2 preamble
> with no "or later" clause.

It was unintentional.  Poked upstream, they fixed it, new tarball.

> > glib2 also provides some gtk-doc, and while that's currently stored
> > in glib2-doc it has switched around in the past between -devel (which
> > is a BuildRequire) and -docs so I'm more comfortable explicitly
> > owning the relevant dirs than all of them.
> 
> There is no reason to be concerned, because we install into an empty
> %{buildroot}, and that one won't include any docs from glib2 build
> requirements.
> 
> > %dir %{_datadir}/gtk-doc
> > %dir %{_datadir}/gtk-doc/html
> > %dir %{_datadir}/gtk-doc/html/libmbim-glib
> > %{_datadir}/gtk-doc/html/libmbim-glib/*
> 
>   %{_datadir}/gtk-doc/
> 
> achieves exactly the same thing.

Fixed.

spec and RPM in same location again.  Thanks!

Spec URL: http://people.redhat.com/dcbw/libmbim.spec
SRPM URL: http://people.redhat.com/dcbw/libmbim-1.5.0-1.20130815git.fc20.src.rpm

[...]

The diff against previous package looks almost good.

With the new snapshot, the license for the utils package is GPLv2+ now actually, not "GPLv2 and GPLv2+".


> The documentation is pre-generated ...

Oh! That should be explained in the spec file together with info on how to reproduce the git snapshot:

  https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control

The timestamps on the html doc files get refreshed during the %install step. Some packagers try to fight that using "install -p" with either

  make install INSTALL="install -p" DESTDIR=$RPM_BUILD_ROOT
or
  %make_install INSTALL="install -p"

but that doesn't affect many files in this case.


Just to be sure, with the latest stable release being 1.4.0, it seems the next official release following these 1.5.0 snapshots will be 1.6.0. Else the pre-release versioning scheme would be applicable:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

[...]

That's no major issues that require another update for this review request. It's more convenient to apply remaining fixes/changes in Fedora package git.

APPROVED

New Package SCM Request
=======================
Package Name: libmbim
Short Description: Support library for the Mobile Broadband Interface Model protocol
Owners: dcbw
Branches: f20+
InitialCC:

Git done (by process-git-requests).

Imported and built, thanks everyone!