Bug 969209 - Review Request: nx-libs - NX X11 protocol compression libraries
Review Request: nx-libs - NX X11 protocol compression libraries
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jon Ciesla
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 969212 969220 998551
  Show dependency treegraph
 
Reported: 2013-05-30 17:58 EDT by Orion Poplawski
Modified: 2013-10-19 17:12 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-09-03 11:57:04 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
limburgher: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
Initial review (48.59 KB, text/plain)
2013-06-01 06:46 EDT, Mario Ceresa
no flags Details

  None (edit)
Description Orion Poplawski 2013-05-30 17:58:05 EDT
Spec URL: http://www.cora.nwra.com/~orion/fedora/nx/nx-libs.spec
SRPM URL: http://www.cora.nwra.com/~orion/fedora/nx/nx-libs-3.5.0.20-2.fc18.src.rpm

Description:
NX is a software suite which implements very efficient compression of
the X11 protocol. This increases performance when using X
applications over a network, especially a slow one.

This package provides the core nx-X11 libraries customized for
nxagent/x2goagent.

Fedora Account System Username: orion

This is part of this feature: https://fedoraproject.org/wiki/Features/X2Go
Comment 1 Mario Ceresa 2013-05-31 08:47:46 EDT
Thanks Orion, for pursuing x2go packaging!

I had the following errors trying to mock build it with fedora-review:

../../config/makedepend/makedepend: warning:  imake.c (reading /usr/include/bits/byteswap-16.h), line 20: # error "Never use <bits/byteswap-16.h> directly; include <byteswap.h> instead."
../../config/makedepend/makedepend: warning:  makestrs.c (reading /usr/include/bits/byteswap-16.h), line 20: # error "Never use <bits/byteswap-16.h> directly; include <byteswap.h> instead."
../../config/makedepend/makedepend: warning:  AuDispose.c (reading /usr/include/bits/byteswap-16.h), line 20: # error "Never use <bits/byteswap-16.h> directly; include <byteswap.h> instead."
../../config/makedepend/makedepend: warning:  A8Eq.c (reading /usr/include/bits/byteswap-16.h), line 20: # error "Never use <bits/byteswap-16.h> directly; include <byteswap.h> instead."
/builddir/build/BUILD/nx-libs-3.5.0.20/my_configure: line 8: syntax error near unexpected token `./nx-X11/lib/Xft/config.guess'
Comment 2 Orion Poplawski 2013-05-31 11:29:06 EDT
Ah, new configure macro in rawhide was causing issues.  Fixed.
 
http://www.cora.nwra.com/~orion/fedora/nx/nx-libs.spec
http://www.cora.nwra.com/~orion/fedora/nx/nx-libs-3.5.0.20-3.fc18.src.rpm

* Fri May 31 2013 Orion Poplawski <orion@cora.nwra.com> - 3.5.0.20-3
- Fix quoting when creating my_configure script
Comment 3 Mario Ceresa 2013-06-01 06:44:15 EDT
Okay, I point out some potential issues:

* mock fails to install the package:

Error: Package: x2goagent-3.5.0.20-3.fc20.x86_64 (/x2goagent-3.5.0.20-3.fc20.x86_64)
Requires: x2goserver

* rpmlint is not silent:

- Several non standard executable permission (0775L) on libraries. Not sure if it's intentional
- Several only-non-binary-in-usr-lib
- Several devel-file-in-non-devel-package
- Others: 

nx-libs.x86_64: W: self-obsoletion nx <= 3.5.0.20-3.fc20 obsoletes nx = 3.5.0.20-3.fc20
nx-libs.x86_64: W: self-obsoletion nx(x86-64) <= 3.5.0.20-3.fc20 obsoletes nx(x86-64) = 3.5.0.20-3.fc20
nx-libs.x86_64: E: no-binary
nx-libs.x86_64: W: non-conffile-in-etc /etc/ld.so.conf.d/nx-libs-x86_64.conf
nx-libs.x86_64: E: incorrect-fsf-address /usr/share/doc/nx-libs-3.5.0.20/LICENSE
libXcomp.x86_64: E: missing-call-to-setgroups /usr/lib64/nx/libXcomp.so.3.5.0
libXcomp.x86_64: E: incorrect-fsf-address /usr/share/doc/libXcomp-3.5.0.20/LICENSE
libXcompext.x86_64: E: incorrect-fsf-address /usr/share/doc/libXcompext-3.5.0.20/LICENSE
libXcompshad.x86_64: E: incorrect-fsf-address /usr/share/doc/libXcompshad-3.5.0.20/LICENSE
nx-devel.x86_64: W: no-dependency-on nx/nx-libs/libnx
nx-fontconfig-devel.x86_64: W: no-dependency-on nx-fontconfig/nx-fontconfig-libs/libnx-fontconfig
nx-freetype2-devel.x86_64: W: no-dependency-on nx-freetype2/nx-freetype2-libs/libnx-freetype2
nxagent.x86_64: E: missing-call-to-setgroups /usr/lib64/nx/bin/nxagent
x2goagent.x86_64: W: dangling-relative-symlink /usr/lib64/x2go/bin/x2goagent ../../nx/bin/nxagent

* there are unversioned so-files:
libNX_X11: /usr/lib64/nx/X11/libximcp.so
libNX_X11: /usr/lib64/nx/X11/libxlcDef.so
libNX_X11: /usr/lib64/nx/X11/libxlcUTF8Load.so
libNX_X11: /usr/lib64/nx/X11/libxlibi18n.so
libNX_X11: /usr/lib64/nx/X11/libxlocale.so
libNX_X11: /usr/lib64/nx/X11/libxomGeneric.so

However, they seem private, so maybe it is not an issue

I'll also attach the fedora-review output if you need it as a reference
Comment 4 Mario Ceresa 2013-06-01 06:46:07 EDT
Created attachment 755522 [details]
Initial review
Comment 5 Orion Poplawski 2013-06-12 18:00:53 EDT
(In reply to Mario Ceresa from comment #3)
> Okay, I point out some potential issues:
> 
> * mock fails to install the package:
> 
> Error: Package: x2goagent-3.5.0.20-3.fc20.x86_64
> (/x2goagent-3.5.0.20-3.fc20.x86_64)
> Requires: x2goserver

x2goserver and x2goagent each require each other at run-time, so this is expected.

> * rpmlint is not silent:
> 
> - Several non standard executable permission (0775L) on libraries. Not sure
> if it's intentional

Nope, fixed.

> - Several only-non-binary-in-usr-lib

Bug in rpmlint, see bug 483199

> - Several devel-file-in-non-devel-package

Fixed

> - Others: 
> 
> nx-libs.x86_64: W: self-obsoletion nx <= 3.5.0.20-3.fc20 obsoletes nx =
> 3.5.0.20-3.fc20
> nx-libs.x86_64: W: self-obsoletion nx(x86-64) <= 3.5.0.20-3.fc20 obsoletes
> nx(x86-64) = 3.5.0.20-3.fc20

Fixed

> nx-libs.x86_64: E: no-binary

nx-libs is kind of just a container.

> nx-libs.x86_64: W: non-conffile-in-etc /etc/ld.so.conf.d/nx-libs-x86_64.conf

Fixed.

> nx-libs.x86_64: E: incorrect-fsf-address
> /usr/share/doc/nx-libs-3.5.0.20/LICENSE

Fixed.

> libXcomp.x86_64: E: missing-call-to-setgroups /usr/lib64/nx/libXcomp.so.3.5.0

Hmm, this is an odd one...

> nx-devel.x86_64: W: no-dependency-on nx/nx-libs/libnx

Fixed.

> nx-fontconfig-devel.x86_64: W: no-dependency-on
> nx-fontconfig/nx-fontconfig-libs/libnx-fontconfig
> nx-freetype2-devel.x86_64: W: no-dependency-on
> nx-freetype2/nx-freetype2-libs/libnx-freetype2

Fixed the naming.

> nxagent.x86_64: E: missing-call-to-setgroups /usr/lib64/nx/bin/nxagent

Ah, okay, looking into it.

> x2goagent.x86_64: W: dangling-relative-symlink /usr/lib64/x2go/bin/x2goagent
> ../../nx/bin/nxagent
> 
> * there are unversioned so-files:
> libNX_X11: /usr/lib64/nx/X11/libximcp.so
> libNX_X11: /usr/lib64/nx/X11/libxlcDef.so
> libNX_X11: /usr/lib64/nx/X11/libxlcUTF8Load.so
> libNX_X11: /usr/lib64/nx/X11/libxlibi18n.so
> libNX_X11: /usr/lib64/nx/X11/libxlocale.so
> libNX_X11: /usr/lib64/nx/X11/libxomGeneric.so
> 
> However, they seem private, so maybe it is not an issue

These are links to versioned files.


http://www.cora.nwra.com/~orion/fedora/nx/nx-libs.spec
http://www.cora.nwra.com/~orion/fedora/nx/nx-libs-3.5.0.20-4.fc18.src.rpm

* Tue Jun 11 2013 Orion Poplawski <orion@cora.nwra.com> - 3.5.0.20-4
- Fix 775 library permissions
- Move nx/X11 .so files to -devel
- Fix nx obsoletes
- Mark ld.so.conf.d files config(noreplace)
- Fix requires
Comment 6 Mario Ceresa 2013-06-13 04:18:54 EDT
Hello Orion!
here there is another round of questions:

* rpmlint:
x2goagent.x86_64: W: dangling-relative-symlink /usr/lib64/x2go/bin/x2goagent ../../nx/bin/nxagent

* has any of the nx packages ever been granted an FPC exception for all the libraries it bundles? I couldn't find any ticket around.

* Not sure about this: "Fully versioned dependency in subpackages, if present."
Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in libNX_ICE-devel , libNX_SM-devel , libNX_X11-devel , libNX_Xau-devel , libNX_Xext-devel , libNX_Xfixes-devel , libNX_Xi-devel , libNX_Xmu-devel , libNX_Xpm-devel , libNX_Xrender-devel , libNX_Xt-devel , libNX_Xv-devel , libNX_fontenc-devel , libNX_xkbfile-devel , libXcomp-devel , libXcompext-devel , libXcompshad-devel , libNX_Mesa-devel , nx-bitmaps-devel , nx-devel , libNX_fontconfig-devel , libNX_freetype-devel , nx-proto-devel , nxagent , nxauth , nxproxy , x2goagent
Comment 7 Orion Poplawski 2013-06-13 19:00:43 EDT
(In reply to Mario Ceresa from comment #6)
> Hello Orion!
> here there is another round of questions:
> 
> * rpmlint:
> x2goagent.x86_64: W: dangling-relative-symlink /usr/lib64/x2go/bin/x2goagent
> ../../nx/bin/nxagent

Okay because x2goagent requires nxagent.

> * has any of the nx packages ever been granted an FPC exception for all the
> libraries it bundles? I couldn't find any ticket around.

Hmm, I never realized quite the extent of the bundling - let me poke around there some more.  I've gotten rid of some, but not all.

> * Not sure about this: "Fully versioned dependency in subpackages, if
> present."
> Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in
> libNX_ICE-devel , libNX_SM-devel , libNX_X11-devel , libNX_Xau-devel ,
> libNX_Xext-devel , libNX_Xfixes-devel , libNX_Xi-devel , libNX_Xmu-devel ,
> libNX_Xpm-devel , libNX_Xrender-devel , libNX_Xt-devel , libNX_Xv-devel ,
> libNX_fontenc-devel , libNX_xkbfile-devel , libXcomp-devel ,
> libXcompext-devel , libXcompshad-devel , libNX_Mesa-devel , nx-bitmaps-devel
> , nx-devel , libNX_fontconfig-devel , libNX_freetype-devel , nx-proto-devel
> , nxagent , nxauth , nxproxy , x2goagent

The libN?X*-devel packages require their corresponding runtime versions (which require %{name}%{?_isa} = %{version}-%{release}), although many did not have %{version}-%{release} - fixed.  Not really needed for the program sub-packages.

nx-bitmaps-devel stands on its own (and not sure if it is needed), and so does nx-proto-devel.

Fixed for nx-devel

* Thu Jun 13 2013 Orion Poplawski <orion@cora.nwra.com> - 3.5.0.20-5
- Add more explicit verioned requires
- Drop unnecessary directory ownership by sub-packages
- Remove many bundled libraries


http://www.cora.nwra.com/~orion/fedora/nx/nx-libs.spec
http://www.cora.nwra.com/~orion/fedora/nx/nx-libs-3.5.0.20-5.fc18.src.rpm
Comment 8 Orion Poplawski 2013-07-11 15:29:33 EDT
I've done a lot more cleanup.

* Thu Jul 11 2013 Orion Poplawski <orion@cora.nwra.com> - 3.5.0.20-6
- Drop building and/or shipping a bunch of unneeded libraries

http://www.cora.nwra.com/~orion/fedora/nx/nx-libs.spec
http://www.cora.nwra.com/~orion/fedora/nx/nx-libs-3.5.0.20-6.fc11.src.rpm

I'm still probably shipping more than is strictly needed, but I'd like to err to that side for now.
Comment 9 Christopher Meng 2013-07-21 23:04:13 EDT
I think Orion knows that error of missing-call-to-setgroups.

missing-call-to-setgroups has been renamed to missing-call-to-setgroups-before-setuid.

This will be available in the next version.

And the explanation is:

This executable is calling setuid and setgid without setgroups or initgroups.
There is a high probability this mean it didn't relinquish all groups, and this
would be a potential security issue to be fixed. Seek POS36-C on the web for
details about the problem.

Ref POS36-C:

https://www.securecoding.cert.org/confluence/display/seccode/POS36-C.+Observe+correct+revocation+order+while+relinquishing+privileges
Comment 10 Eric Smith 2013-07-24 17:21:12 EDT
The SRPM link in comment 8 appears to be wrong or dead. It appears that it should be .fc19 rather than .fc11.
Comment 11 Orion Poplawski 2013-07-24 19:23:51 EDT
Oops, yeah:

http://www.cora.nwra.com/~orion/fedora/nx/nx-libs-3.5.0.20-6.fc19.src.rpm

I'm trying to get some info out of upstream.  I'm not sure why it is even calling setuid/setgid in the first place.
Comment 12 Orion Poplawski 2013-08-05 19:08:10 EDT
We're making great progress reviewing the dependencies :), but not nx-libs itself :(.  Could someone step up and take on this review?  I'm also not sure what is a blocker at this point.  Thanks!
Comment 13 Mario Ceresa 2013-08-06 07:49:57 EDT
Orion, I'll be back next Monday, if that's okay to you, tell me and I'll take the review.

Best,

Mario
Comment 14 Orion Poplawski 2013-08-16 12:24:25 EDT
I would just like *somebody* to take the review. :)
Comment 15 Orion Poplawski 2013-08-22 11:19:55 EDT
Well, looks like we've missed the change deadline for F20.  Sorry.
Comment 16 Orion Poplawski 2013-08-28 19:13:27 EDT
Upstream's comment on the setgid issue:

Everything in NX runs under the user who launches the X2Go session. IMHO resetting the effective GID prevents us from setgid file permission manipulations, so that the effective group ID always is the primary/real group ID of the current user that is executing the NX binary.

----

We're rapidly approaching the change deadline for F20.  Are there any blockers in this package?  It would be nice to get a review done soon.
Comment 17 Orion Poplawski 2013-08-29 13:30:19 EDT
http://www.cora.nwra.com/~orion/fedora/nx/nx-libs-3.5.0.20-7.fc19.src.rpm


* Thu Aug 29 2013 Orion Poplawski <orion@cora.nwra.com> - 3.5.0.20-7
- Add patch to call initgroups()
Comment 18 Jon Ciesla 2013-08-30 09:25:27 EDT
I'll give this a go.
Comment 19 Jon Ciesla 2013-08-30 09:43:36 EDT
- rpmlint checks return:

nx-libs.spec:16: W: macro-in-comment %{name}
There is a unescaped macro after a shell style comment in the specfile. Macros
are expanded everywhere, so check if it can cause a problem in this case and
escape the macro with another leading % if appropriate.

nx-libs.spec:493: W: macro-in-comment %{_libdir}
There is a unescaped macro after a shell style comment in the specfile. Macros
are expanded everywhere, so check if it can cause a problem in this case and
escape the macro with another leading % if appropriate.

nx-libs.spec:493: W: macro-in-comment %{_libexedir}
There is a unescaped macro after a shell style comment in the specfile. Macros
are expanded everywhere, so check if it can cause a problem in this case and
escape the macro with another leading % if appropriate.

W: only-non-binary-in-usr-lib  for the -devel pacakges.  Probably OK for NX.

x-libs.x86_64: E: no-binary
The package should be of the noarch architecture because it doesn't contain
any binaries.

Lots of spurious executable perms in debuginfo, you can probably use find to chmod -x in setup.

Would making this noarch overly complicate things?

And lots of bogus spelling error / no docs/manpages.

- package meets naming guidelines
- package meets packaging guidelines
- license ( GPLv2+ ) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files
! post/postun ldconfig ok   Runs on post, but not postun
! devel requires base package n-v-r    -devel pacakges don't Require main lib pacakges

So it look like just commented macros, spurious executables, possibly noarch main package, postun ldconfig, and -devel Requires.
Comment 20 Jon Ciesla 2013-08-30 10:08:49 EDT
Also, after i replaced nx with nx-libs, I had to manually nxproxy before remmina would connect, but then it was fine, so nx-libs should require nxproxy.

I also replaced nx on my freenx server, and that worked, once nxagent and its deps were installed, so it should be required as well.
Comment 21 Orion Poplawski 2013-08-30 23:30:53 EDT
(In reply to Jon Ciesla from comment #19)
> - rpmlint checks return:
>
> x-libs.x86_64: E: no-binary
> The package should be of the noarch architecture because it doesn't contain
> any binaries.
> 
> Lots of spurious executable perms in debuginfo, you can probably use find to
> chmod -x in setup.

Yowza.  Fixed.

> Would making this noarch overly complicate things?

Cannot make the main package noarch, sub-packages arch.

> ! post/postun ldconfig ok   Runs on post, but not postun

Fixed.

> ! devel requires base package n-v-r    -devel pacakges don't Require main
> lib pacakges

Fixed by renaming nx-devel to nx-libs-devel

> So it look like just commented macros, spurious executables, possibly noarch
> main package, postun ldconfig, and -devel Requires.

http://www.cora.nwra.com/~orion/fedora/nx/nx-libs-3.5.0.21-1.fc19.src.rpm

* Fri Aug 30 2013 Orion Poplawski <orion@cora.nwra.com> - 3.5.0.21-1
- Update to 3.5.0.21
- Many bundled libs removed upstream
- Drop initgroups patch applied upstream
- Fix macro in comments
- Remove execute permissions from source files
- Add %%postun ldconfig scripts
- Rename nx-devel nx-libs-devel
Comment 22 Orion Poplawski 2013-08-30 23:37:02 EDT
(In reply to Jon Ciesla from comment #20)
> Also, after i replaced nx with nx-libs, I had to manually nxproxy before
> remmina would connect, but then it was fine, so nx-libs should require
> nxproxy.

Actually, remmina-nx needs to require nxproxy once this is imported.

> I also replaced nx on my freenx server, and that worked, once nxagent and
> its deps were installed, so it should be required as well.

And again, freenx-server will need to require nxagent.

The problem is that the current nx package contains both server and client components, while this nx-libs package separates them out.  As such we can't really automatically migrate the dependencies properly.
Comment 23 Jon Ciesla 2013-09-03 09:16:53 EDT
Looks great, but I'm not sure I agree about the requires.  If this is meant to be a truly drop-in replacement, my expectation would be that upgrading from the old nx to this would Just Work, and I'd think this could be achieved with the right Provides and Requires.  But that's your call.

APPROVED.
Comment 24 Orion Poplawski 2013-09-03 11:24:47 EDT
Okay, I have nx-libs, nxproxy, and nxagent all obsolete/provide nx so current upgrades will be drop in.  Then we can tweak the requires of remmina-plugins-nx and freenx-server later.

* Mon Sep 3 2013 Orion Poplawski <orion@cora.nwra.com> - 3.5.0.21-2
- Have nxagent and nxproxy also obsolete/provide nx


New Package SCM Request
=======================
Package Name: nx-libs
Short Description: NX X11 protocol compression libraries
Owners: orion
Branches: f18 f19 f20 el6
InitialCC:
Comment 25 Jon Ciesla 2013-09-03 11:27:43 EDT
Git done (by process-git-requests).
Comment 26 Orion Poplawski 2013-09-03 11:57:04 EDT
Checked in and built.  Thanks all.

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