Bug 1228924

I will take this review.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


Issues:
=======
- Almost the entire distribution is GPLv2+, but libtools/sjson.h is GPLv3+,
  which makes the binary package GPLv3+ as well.

- The package uses two directories that it does not own:
    %{_libdir}/girepository-1.0
    %{_datadir}/gir-1.0
  The package needs to either own those directories or Requires: some package
  that owns them.

- The build runs in quiet mode, which makes verifying the compiler flags
  hard.  Please consider either configuring with --disable-silent-rules,
  or building with make V=1.

- You can save time building a static archive and then deleting it in %install
  by passing --disable-static to %configure.

- The build log shows two complaints from the debuginfo generator:

cpio: megatools-1.9.95/sjson.c: Cannot stat: No such file or directory
cpio: megatools-1.9.95/sjson.gen.c: Cannot stat: No such file or directory

  You can get slightly better debuginfo by adding this to the end of %install:

  ln -s libtools/sjson.gen.c .

  Unfortunately, the original sjson.c does not appear to be available.

- rpmlint finds binary-or-shlib-defines-rpath problems with the binaries,
  which apparently means the "find ... chrpath" invocation in %install didn't
  work.  I don't know why not, but these approaches work:
  https://fedoraproject.org/wiki/Packaging:Guidelines#Removing_Rpath

  But see the next item.

- The unused-direct-shlib-dependency problems can usually be fixed by adding
  -Wl,--as-needed to LDFLAGS.  Unfortunately, libtool cleverly reorders link
  flags and puts -Wl,--as-needed *after* the extraneous libraries, rendering
  it useless.  I do this after %configure in several of my packages to solve
  both the previous problem and this one:

# Get rid of undesirable hardcoded runpaths; workarouond libtool reordering
# -Wl,--as-needed after all the libraries.
sed -e 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' \
    -e 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' \
    -e 's|CC="\(g.*\)"|CC="\1 -Wl,--as-needed"|' \
    -i libtool

  That still leaves a direct unused dependency from the library on
  libpthread.so.0, so apparently the library does not need to be linked with
  the -pthread option.

- Since TODO is an empty file, there is not much point including it in %doc.

- Consider adding a %check script.  I see a couple of binaries in tests
  (test-aes and test-rsa), as well as a handful of scripts.

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[!]: Rpath absent or only used for internal libs.
     Note: See rpmlint output
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "GPL (v2 or later)", "GPL (v3 or later)", "Unknown or
     generated". 13 files have unknown license. Detailed output of
     licensecheck in /home/jamesjer/1228924-megatools/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib64/girepository-1.0,
     /usr/share/gir-1.0
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 40960 bytes in 5 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: megatools-1.9.95-1.fc23.x86_64.rpm
          megatools-devel-1.9.95-1.fc23.x86_64.rpm
          megatools-1.9.95-1.fc23.src.rpm
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megals ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megadl ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megadf ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megaput ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megafs ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megaget ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megamkdir ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megareg ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megacopy ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megarm ['/usr/lib64']
megatools.x86_64: E: zero-length /usr/share/doc/megatools/TODO
megatools-devel.x86_64: W: only-non-binary-in-usr-lib
megatools-devel.x86_64: W: no-documentation
megatools.src: W: spelling-error %description -l en_US megareg -> mega reg, mega-reg, megastar
3 packages and 0 specfiles checked; 11 errors, 3 warnings.




Rpmlint (debuginfo)
-------------------
Checking: megatools-debuginfo-1.9.95-1.fc23.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
megatools-devel.x86_64: W: only-non-binary-in-usr-lib
megatools-devel.x86_64: W: no-documentation
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megals ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megadl ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megadf ['/usr/lib64']
megatools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmega.so.0.0.0 /lib64/libssl.so.10
megatools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmega.so.0.0.0 /lib64/libcurl.so.4
megatools.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmega.so.0.0.0 /lib64/libpthread.so.0
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megaput ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megafs ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megaget ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megamkdir ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megareg ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megacopy ['/usr/lib64']
megatools.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/megarm ['/usr/lib64']
megatools.x86_64: E: zero-length /usr/share/doc/megatools/TODO
3 packages and 0 specfiles checked; 11 errors, 5 warnings.



Requires
--------
megatools-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    libmega.so.0()(64bit)
    megatools(x86-64)
    pkgconfig(gio-2.0)
    pkgconfig(libcurl)
    pkgconfig(openssl)

megatools (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libc.so.6()(64bit)
    libcrypto.so.10()(64bit)
    libcrypto.so.10(libcrypto.so.10)(64bit)
    libcurl.so.4()(64bit)
    libfuse.so.2()(64bit)
    libfuse.so.2(FUSE_2.6)(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.3.1)(64bit)
    libgio-2.0.so.0()(64bit)
    libglib-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libmega.so.0()(64bit)
    libpthread.so.0()(64bit)
    libssl.so.10()(64bit)
    libssl.so.10(libssl.so.10)(64bit)
    rtld(GNU_HASH)



Provides
--------
megatools-devel:
    megatools-devel
    megatools-devel(x86-64)
    pkgconfig(libmega)

megatools:
    libmega.so.0()(64bit)
    megatools
    megatools(x86-64)



Source checksums
----------------
http://megatools.megous.com/builds/megatools-1.9.95.tar.gz :
  CHECKSUM(SHA256) this package     : a46a560c8769b40f073fd27b321d6b89f8ac0f0ca73e6ed83047c2619fe6b437
  CHECKSUM(SHA256) upstream package : a46a560c8769b40f073fd27b321d6b89f8ac0f0ca73e6ed83047c2619fe6b437
http://megatools.megous.com/builds/megatools-1.9.95.tar.gz.asc :
  CHECKSUM(SHA256) this package     : e001e7e0319229d03acb7ce08257f128b75cd59ddb4817cc6f7d3e5ac0c4fc00
  CHECKSUM(SHA256) upstream package : e001e7e0319229d03acb7ce08257f128b75cd59ddb4817cc6f7d3e5ac0c4fc00


Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20
Command line :/usr/bin/fedora-review -b 1228924 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

(In reply to Jerry James from comment #2)
> Issues:
> =======
> - Almost the entire distribution is GPLv2+, but libtools/sjson.h is GPLv3+,
>   which makes the binary package GPLv3+ as well.
You're correct... I'll make the change.

> 
> - The package uses two directories that it does not own:
>     %{_libdir}/girepository-1.0
>     %{_datadir}/gir-1.0
>   The package needs to either own those directories or Requires: some package
>   that owns them.
Missed that... OK... 

> 
> - The build runs in quiet mode, which makes verifying the compiler flags
>   hard.  Please consider either configuring with --disable-silent-rules,
>   or building with make V=1.
OK

> 
> - You can save time building a static archive and then deleting it in
> %install
>   by passing --disable-static to %configure.
OK

> 
> - The build log shows two complaints from the debuginfo generator:
> 
> cpio: megatools-1.9.95/sjson.c: Cannot stat: No such file or directory
> cpio: megatools-1.9.95/sjson.gen.c: Cannot stat: No such file or directory
I'll look into it.

> 
>   You can get slightly better debuginfo by adding this to the end of
> %install:
> 
>   ln -s libtools/sjson.gen.c .
> 
>   Unfortunately, the original sjson.c does not appear to be available.
Will investigate.

> 
> - rpmlint finds binary-or-shlib-defines-rpath problems with the binaries,
>   which apparently means the "find ... chrpath" invocation in %install didn't
>   work.  I don't know why not, but these approaches work:
>   https://fedoraproject.org/wiki/Packaging:Guidelines#Removing_Rpath
> 
>   But see the next item.
> 
> - The unused-direct-shlib-dependency problems can usually be fixed by adding
>   -Wl,--as-needed to LDFLAGS.  Unfortunately, libtool cleverly reorders link
>   flags and puts -Wl,--as-needed *after* the extraneous libraries, rendering
>   it useless.  I do this after %configure in several of my packages to solve
>   both the previous problem and this one:
> 
> # Get rid of undesirable hardcoded runpaths; workarouond libtool reordering
> # -Wl,--as-needed after all the libraries.
> sed -e 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' \
>     -e 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' \
>     -e 's|CC="\(g.*\)"|CC="\1 -Wl,--as-needed"|' \
>     -i libtool
> 
>   That still leaves a direct unused dependency from the library on
>   libpthread.so.0, so apparently the library does not need to be linked with
>   the -pthread option.
Yeah, I was thinking the chrpath would handle that.  I'll review.

> 
> - Since TODO is an empty file, there is not much point including it in %doc.
I'll remove it.
> 
> - Consider adding a %check script.  I see a couple of binaries in tests
>   (test-aes and test-rsa), as well as a handful of scripts.
I'll look into it.
>

(In reply to Gerald Cox from comment #3)
> 
> > 
> > - The package uses two directories that it does not own:
> >     %{_libdir}/girepository-1.0
> >     %{_datadir}/gir-1.0
> >   The package needs to either own those directories or Requires: some package
> >   that owns them.
> Missed that... OK... 
> 
Just checked and those directories I believe are owned by gobject-introspection, which is in the BuildRequires.

Location of source files:

Spec URL: https://gbcox.fedorapeople.org/specs/megatools.spec
SRPM URL: https://gbcox.fedorapeople.org/copr/megatools-1.9.95-2.fc22.src.rpm

OK Jerry, I've made the changes and uploaded the new spec and srpm.

I had a discussion with upstream:  sjson.c isn't used by the build.  sjson.gen.c has references to sjson.c in line comments and that is apparently why it is being picked up.  

Regarding libpthread it is pulled in by glib2 (via pkg-config), not by megatools themselves, and it is used.

See comment #4 about ownership of gir* directories.

Thanks so much for your recommendation for the unused-direct-shlib-dependency problems - that resolved the error messages.

(In reply to Gerald Cox from comment #4)
> Just checked and those directories I believe are owned by
> gobject-introspection, which is in the BuildRequires.

Yes, but it isn't in the Requires, which means that if this package is installed, but gobject-introspection is not, and this package is subsequently removed, those directories will stay behind.  Either this package must also own those directories, or it needs to Requires: gobject-introspection.

(In reply to Gerald Cox from comment #5)
> I had a discussion with upstream:  sjson.c isn't used by the build. 
> sjson.gen.c has references to sjson.c in line comments and that is
> apparently why it is being picked up.  

Right, the debuginfo generator wants to see it, since sjson.gen.c refers to it.  But that isn't critical.

> Regarding libpthread it is pulled in by glib2 (via pkg-config), not by
> megatools themselves, and it is used.

Well, I see -pthread in the list of flags passed to gcc when the library is linked.  Even if glib2 itself uses pthreads, apparently the library itself does not, which is why this is being marked as an unused direct dependency.  This isn't critical enough to lose sleep over, though.  On the other hand, it can be fixed by doing this between %configure and make:

sed -i 's/\(GLIB_CFLAGS = \)-pthread/\1/' Makefile

which appears to mean that this is a glib2 packaging bug.  You can do that or not, at your discretion.

> Thanks so much for your recommendation for the
> unused-direct-shlib-dependency problems - that resolved the error messages.

No problem.

It looks like removing the rpath so early in the build process is now causing another issue, though.  The Rawhide build is now failing when trying to link the binaries.  I had to add this to the spec file right before the make invocation to review this version:

export LD_LIBRARY_PATH=$PWD/.libs

Also, I found that the these errors:

./libtool: line 7905: func_munge_path_list: command not found

can be gotten rid of by running "autoreconf -fi" in %prep.  Your choice whether you want to do that or not.

Location of source files:

Spec URL: https://gbcox.fedorapeople.org/specs/megatools.spec
SRPM URL: https://gbcox.fedorapeople.org/copr/megatools-1.9.95-3.fc22.src.rpm

Thanks Jerry!  I went ahead and incorporated your optional recommendations.  

Regarding REQUIRES:
I actually ran into this issue on another package.  I was adding the packages that were referenced in the BUILDREQUIRES.  It was pointed out to me that this was incorrect.  Check out:
http://fedoraproject.org/wiki/Packaging:Guidelines#Requires
http://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires 
The key quote:  
"Packages must not contain unnecessary explicit Requires on libraries. We generally rely on rpmbuild to automatically add dependencies on library SONAMEs. Modern package management tools are capable of resolving such dependencies to determine the required packages in many cases."

(In reply to Gerald Cox from comment #7)
> Location of source files:
> 
> Spec URL: https://gbcox.fedorapeople.org/specs/megatools.spec
> SRPM URL: https://gbcox.fedorapeople.org/copr/megatools-1.9.95-3.fc22.src.rpm
> 
> Thanks Jerry!  I went ahead and incorporated your optional recommendations.  

No problem.

> Regarding REQUIRES:
> I actually ran into this issue on another package.  I was adding the
> packages that were referenced in the BUILDREQUIRES.  It was pointed out to
> me that this was incorrect.  Check out:
> http://fedoraproject.org/wiki/Packaging:Guidelines#Requires
> http://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires 
> The key quote:  
> "Packages must not contain unnecessary explicit Requires on libraries. We
> generally rely on rpmbuild to automatically add dependencies on library
> SONAMEs. Modern package management tools are capable of resolving such
> dependencies to determine the required packages in many cases."

Yes, for libraries that is correct.  However, the gobject-introspection package is not a library, so dependencies on it are not auto-generated.  Take a look at the list of Requires in comment 2.  It isn't on that list.

I don't think you want to make that a dependency anyway.  I think you should make your package own those directories.  If you check with rpm -qf, you'll see that many other packages have already done exactly this.

(In reply to Jerry James from comment #8)
> 
> Yes, for libraries that is correct.  However, the gobject-introspection
> package is not a library, so dependencies on it are not auto-generated. 
> Take a look at the list of Requires in comment 2.  It isn't on that list.
> 
> I don't think you want to make that a dependency anyway.  I think you should
> make your package own those directories.  If you check with rpm -qf, you'll
> see that many other packages have already done exactly this.

Yep, when I read your comments on oggify, I realized probably a misunderstanding on my part.  I'll make the change.  Thanks again!

Location of source files:

Spec URL: https://gbcox.fedorapeople.org/specs/megatools.spec
SRPM URL: https://gbcox.fedorapeople.org/copr/megatools-1.9.95-4.fc22.src.rpm

OK, here is the latest review candidate, incorporating the ownership change.
Thanks!

Everything looks great now.  This package is APPROVED.

Great!  Thanks Jerry!

New Package SCM Request
=======================
Package Name: megatools
Short Description: Command line client for accessing Mega service
Upstream URL: http://megatools.megous.com/
Owners: gbcox
Branches: f22 f23
InitialCC:

Git done (by process-git-requests).

megatools-1.9.95-4.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/megatools-1.9.95-4.fc22

Rawhide, F22 Builds complete.
Submitted to Bodhi for F22.

megatools-1.9.95-4.fc22 has been pushed to the Fedora 22 stable repository.