Bug 1194212 - Review Request: compat-libuv010 - Platform layer for node.js - compatibility library for nodejs 0.10.x
Summary: Review Request: compat-libuv010 - Platform layer for node.js - compatibility ...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Stephen Gallagher
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1117992
TreeView+ depends on / blocked
 
Reported: 2015-02-19 10:48 UTC by T.C. Hollingsworth
Modified: 2015-02-20 00:13 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-02-19 20:52:47 UTC
Type: ---
Embargoed:
sgallagh: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description T.C. Hollingsworth 2015-02-19 10:48:33 UTC
Spec: https://patches.fedorapeople.org/compat-libuv010/compat-libuv010.spec
SRPM: https://patches.fedorapeople.org/compat-libuv010/compat-libuv010-0.10.33-2.fc20.src.rpm
FAS: patches
Description:
Compatibility libuv library for nodejs 0.10.x.

--

This is needed to update libuv to the latest version for other applications in Fedora 22, in advance of being able to update to node.js 0.12 for F23.

Comment 1 Stephen Gallagher 2015-02-19 13:44:41 UTC
tl;dr: One minor thing to fix up before approval.


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

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


===== Issues =====
* No Requires: %{name}%{?_isa} = %{version}-%{release} in compat-
     libuv010-devel


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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[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]: Rpath absent or only used for internal libs.

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.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "MIT/X11 (BSD like)", "ISC", "Unknown or generated", "BSD (2 clause)". 2
     files have unknown license. Detailed output of licensecheck in
     /dev/shm/1194212-compat-libuv010/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
[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.
     Note: Test run failed
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Test run failed
[x]: Packages must not store files under /srv, /opt or /usr/local
     Note: Test run failed
[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]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[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]: 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 do not use a name that already exist
[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.

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

Generic:
[-]: 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).
[!]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in compat-
     libuv010-devel
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Scriptlets must be sane, if used.
[-]: 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]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[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:
[ ]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
     Note: Test run failed
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: compat-libuv010-0.10.33-2.fc21.x86_64.rpm
          compat-libuv010-devel-0.10.33-2.fc21.x86_64.rpm
          compat-libuv010-0.10.33-2.fc21.src.rpm
compat-libuv010.x86_64: W: spelling-error Summary(en_US) js -> dis, ks, j
compat-libuv010.x86_64: W: spelling-error Summary(en_US) nodejs -> nodes, node's
compat-libuv010.x86_64: W: spelling-error %description -l en_US libuv -> Malibu
compat-libuv010.x86_64: W: spelling-error %description -l en_US nodejs -> nodes, node's
compat-libuv010.x86_64: E: missing-call-to-setgroups-before-setuid /usr/lib64/libuv.so.0.10
compat-libuv010-devel.x86_64: W: spelling-error Summary(en_US) libuv -> Malibu
compat-libuv010-devel.x86_64: W: spelling-error Summary(en_US) nodejs -> nodes, node's
compat-libuv010-devel.x86_64: W: spelling-error %description -l en_US libuv -> Malibu
compat-libuv010-devel.x86_64: W: spelling-error %description -l en_US nodejs -> nodes, node's
compat-libuv010-devel.x86_64: W: unexpanded-macro Provides pkgconfig(compat-libuv010) = 0.10.33.git%{git_snapshot} %{git_snapshot}
compat-libuv010-devel.x86_64: W: only-non-binary-in-usr-lib
compat-libuv010-devel.x86_64: W: no-documentation
compat-libuv010.src: W: spelling-error Summary(en_US) js -> dis, ks, j
compat-libuv010.src: W: spelling-error Summary(en_US) nodejs -> nodes, node's
compat-libuv010.src: W: spelling-error %description -l en_US libuv -> Malibu
compat-libuv010.src: W: spelling-error %description -l en_US nodejs -> nodes, node's
3 packages and 0 specfiles checked; 1 errors, 15 warnings.




Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Requires
--------
compat-libuv010 (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libc.so.6()(64bit)
    libdl.so.2()(64bit)
    libgcc_s.so.1()(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    libstdc++.so.6()(64bit)
    rtld(GNU_HASH)

compat-libuv010-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    compat-libuv010
    pkgconfig



Provides
--------
compat-libuv010:
    compat-libuv010
    compat-libuv010(x86-64)
    libuv.so.0.10()(64bit)

compat-libuv010-devel:
    compat-libuv010-devel
    compat-libuv010-devel(x86-64)
    pkgconfig(compat-libuv010)



Source checksums
----------------
http://libuv.org/dist/v0.10.33/libuv-v0.10.33.tar.gz :
  CHECKSUM(SHA256) this package     : a10475d031200fe50923d15e2256e41b1180ed4f26e587317aab9c213c65a162
  CHECKSUM(SHA256) upstream package : a10475d031200fe50923d15e2256e41b1180ed4f26e587317aab9c213c65a162


Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
Command line :/usr/bin/fedora-review -b 1194212
Buildroot used: fedora-21-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

Comment 2 T.C. Hollingsworth 2015-02-19 13:53:09 UTC
Spec: https://patches.fedorapeople.org/compat-libuv010/compat-libuv010.spec
SRPM: https://patches.fedorapeople.org/compat-libuv010/compat-libuv010-0.10.33-3.fc20.src.rpm

* Thu Feb 19 2015 T.C. Hollingsworth <tchollingsworth> - 1:0.10.33-3
- add missing %%{_?isa} to devel requires of main package

--

Fixed in the regular libuv package as well.  :-)

Comment 3 Stephen Gallagher 2015-02-19 13:55:00 UTC
Package review granted. Thanks for playing.

Comment 4 T.C. Hollingsworth 2015-02-19 13:57:42 UTC
New Package SCM Request
=======================
Package Name: compat-libuv010
Short Description: Platform layer for node.js - compatibility library for nodejs 0.10.x
Upstream URL: http://libuv.org/
Owners: patches sgallagh jamielinux
Branches: f22
InitialCC:

Comment 5 Gwyn Ciesla 2015-02-19 15:25:30 UTC
Git done (by process-git-requests).

Comment 6 Michael Schwendt 2015-02-19 16:01:18 UTC
The pkg-config file defines includedir, but doesn't pass it to the compiler.

There's an unexpanded %{git_snapshot} in the .pc file. This has potential to break version queries.

Caution! The -devel package does not contain any *.so file, so one cannot link with -luv as specified in $(pkg-config --libs). How is that supposed to work?

Btw, typically the so-called "compat" packages are runtime only packages with no -devel package to compile/link with. It would have been okay to name it libuv010 and not use the compat- prefix.

Base library packages belong into group "System Environment/Libraries" for many years. Their -devel packages belong into group "Development/Libraries". The group tag is optional nowadays:
https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag

Pkg-config are automatic these days. No need to explicit require "pkgconfig".

Comment 7 Michael Schwendt 2015-02-19 16:11:35 UTC
> Pkg-config are automatic these days.

Make that: Pkg-config dependencies are automatic these days.

Comment 8 T.C. Hollingsworth 2015-02-19 20:20:31 UTC
Well, clearly nobody has ever really used the pkgconfig file we have been shipping with libuv, because it has been horribly broken for years!  :-)

(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #6)
> The pkg-config file defines includedir, but doesn't pass it to the compiler.

Fixed in libuv and compat-libuv010 dist-git.
 
> There's an unexpanded %{git_snapshot} in the .pc file. This has potential to
> break version queries.

Fixed in both dist-gits as well.

> Caution! The -devel package does not contain any *.so file, so one cannot
> link with -luv as specified in $(pkg-config --libs). How is that supposed to
> work?

Yeah you need to pass the full name of the shared object in order to use this compat package.  Fixed in compat-libuv010 dist-git.

> Btw, typically the so-called "compat" packages are runtime only packages
> with no -devel package to compile/link with. It would have been okay to name
> it libuv010 and not use the compat- prefix.

Is there any sort of guideline for this?  I used the compat- prefix to signal to packagers of new libuv-using software that they really shouldn't be using this version.  It only exists for one package in the repository and will be around only for Fedora 22 (and rawhide for a month or two).

> Base library packages belong into group "System Environment/Libraries" for
> many years. Their -devel packages belong into group "Development/Libraries".
> The group tag is optional nowadays:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag

Fixed in both libuv and compat-libuv010.

> Pkg-config are automatic these days. No need to explicit require "pkgconfig".

Is this true for Fedora and RHEL>=5?  I didn't drop it because it's harmless and I wasn't sure which branch might still need it...

http://pkgs.fedoraproject.org/cgit/libuv.git/commit/?id=a2eeb8ee08266fad5eb5a8c458b16736b559e0db
http://pkgs.fedoraproject.org/cgit/compat-libuv010.git/commit/?id=830dc4965b891728ac07cb108f423da5d6ee15f1

Thanks!

Comment 10 Michael Schwendt 2015-02-19 21:08:45 UTC
> Yeah you need to pass the full name of the shared object in order
> to use this compat package.  Fixed in compat-libuv010 dist-git.

Alternatively, it could store the .so symlink outside %_libdir (as to prevent a conflict) and adjust the compiler/linker search via option -L. That's what the .pc file would do also _normally_, i.e. only changing ${libdir} would be necessary:

  libdir=/usr/lib64/compat-libuv010
  Libs: -L${libdir} -luv

Normally, the .pc file would also not relink with -lrt -ldl (from glibc!), because libuv is linked with those already.


[compat packages]

> Is there any sort of guideline for this?

No. It used to be a tradition. Eventually, somebody breaks with the tradition and "abuses" the naming scheme. Then the dist includes bad examples (such as an appended -compat instead of a prefix). Other people base their work on those examples and introduce even more misnamed compat packages (e.g. in the review queue you can meet new packagers, who copy from existing Fedora packages, which don't adhere to the guidelines). At Fedora there is no instance with interest in covering each and every detail such as this.


> RHEL>=5?

RHEL5 is too old. It does not generate Requires for pkg-config inter-dependencies either (i.e. RPM deps for the "Requires:" lines in the .pc files!).

https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_based_on_pkg-config


> I didn't drop it because it's harmless and I wasn't sure which branch
> might still need it...

Well, if this will be released even for EPEL5, you may want to properly replace the old package:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages

That's the only well-defined way to get rid of the old libuv package always. Else it's implementation-dependent whether a depsolver would look for the new libuv package even if nothing needed it yet. Also keep in mind that plain "rpm" evaluates Obsoletes tags when installing/updating packages manually.

Comment 11 T.C. Hollingsworth 2015-02-19 22:09:46 UTC
(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #10)
> Alternatively, it could store the .so symlink outside %_libdir (as to
> prevent a conflict) and adjust the compiler/linker search via option -L.

That would break binary compatibility for every nodejs binary addon module, particularly those end users install with npm.  I really wanted to avoid breaking end-user installed modules without an obvious major version bump in nodejs, when end-users should expect it anyway.

(Unless of course I add it to /etc/ld.so.conf.d/, but then the libuv.so in there might conflict with that of the regular libuv package.)

> No. It used to be a tradition. Eventually, somebody breaks with the
> tradition and "abuses" the naming scheme.

If compat- is not appropriate for this package, it would be nice if there were an alternate prefix that still made it clear that this package is never to be used for building new software.

While we still do need the headers to build nodejs, I didn't want to leave the impression that this package is useful for anything else, like simply naming it libuv010 would do.

> 
> > RHEL>=5?
> 
> RHEL5 is too old. It does not generate Requires for pkg-config
> inter-dependencies either (i.e. RPM deps for the "Requires:" lines in the
> .pc files!).
> 
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#BuildRequires_based_on_pkg-config
> 
> 
> > I didn't drop it because it's harmless and I wasn't sure which branch
> > might still need it...
> 
> Well, if this will be released even for EPEL5, you may want to properly
> replace the old package:

Well, the main libuv package (where many of the pkgconfig bugs you pointed out were copied from wholesale ;-) is present in EPEL5.  But...
  
> https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.
> 2FReplacing_Existing_Packages
> 
> That's the only well-defined way to get rid of the old libuv package always.
> Else it's implementation-dependent whether a depsolver would look for the
> new libuv package even if nothing needed it yet. Also keep in mind that
> plain "rpm" evaluates Obsoletes tags when installing/updating packages
> manually.

...the Obsoletes situation would be the same everywhere, since I would only introduce this package into a branch where I would intend to update the libuv to 1.4.0 like I just did in Rawhide and F22.

And AIUI I should not have this package obsolete libuv, since libuv has another dependent that is compatible with the new version.  (moarvm, which I rebuilt earlier)  Instead, users of the only package that needs this compat library (nodejs) will have to live with a useless libuv package for one release.  (Until Fedora 23 where libuv can Obsolete this when it is no longer needed.)

Regardless of whether or not I add Obsoletes, the upgrade path is preserved, since nodejs' requires on libuv.so.0.10()%{?_isa} will drag in compat-libuv010 now that libuv no longer provides it.

Comment 12 Michael Schwendt 2015-02-20 00:13:19 UTC
> That would break binary compatibility for every nodejs binary
> addon module,

???

Currently, you don't ship any .so symlink at all in the package.

I really only refer to the _symlink_, i.e. a libuv.so that is needed for -luv to work at buildtime. Adding such a file _somewhere_ would certainly not break anything.


> (Unless of course I add it to /etc/ld.so.conf.d/, 

That directory is for runtime, not buildtime.


> If compat- is not appropriate for this package, it would be nice
> if there were an alternate prefix that still made it clear that
> this package is never to be used for building new software.

People happily build with what they find. If you wanted to prevent that, don't ship a -devel package. Which is no solution, if you still want to (re)build existing packages.

Hey, you could make it harder to build with this -devel package. ;-)


> While we still do need the headers to build nodejs, I didn't want
> to leave the impression that this package is useful for anything
> else, like simply naming it libuv010 would do.

The contents matter more than the name. Currently, it _is_ quite an ordinary  parallel-installable library package. (There are no naming guidelines for those either, AFAIK).


> ...the Obsoletes situation would be the same everywhere,

Even more reason to add Obsoletes/Provides as explained in the guidelines.

Imagine what a depsolver needs to do when it finds both the old libuv (providing the same SONAME Provides) and compat-libuv010 in the repo. Behaviour is not well-defined. Developers of package tools have argued quite a bit about what should happen, if e.g. the user runs "yum install something". Yum would take either one it finds, possibly still the shortest package name.

> And AIUI I should not have this package obsolete libuv, 

That's why Obsoletes are versioned. You tell the depsolver that package B replaces package A of versions "less than" something, so the newer package A version is not obsoleted. That's also the way to avoid that the new libuv gets installed when it's not needed by anything.

The current packaging only works out of coincidence, because a tool like Yum eventually notices the new libuv and uses it to replace the old libuv while installing compat-libuv010 to satisfy dependencies. Without Obsoletes, you strictly depend on such implementation behaviour, and you get the new libuv installed even if it will be unused.


> Regardless of whether or not I add Obsoletes, the upgrade path is
> preserved, since nodejs' requires on libuv.so.0.10()%{?_isa} will
> drag in compat-libuv010 now that libuv no longer provides it.

That's the less clean renaming.

It's not the first time that an old package name continues to live on with an incompatible new version of the package software. For example: Qt 4 in "qt" renamed to "qt4" with Qt 5 in "qt". Yes, those packages contain Obsoletes. Not the only example, if one searches a bit.


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