Bug 1334913

Summary: NetworkManager spec file calls rpm to determine ppp version which fails in mock builds
Product: Red Hat Enterprise Linux 7 Reporter: Paul Wouters <pwouters>
Component: NetworkManagerAssignee: Lubomir Rintel <lrintel>
Status: CLOSED ERRATA QA Contact: Desktop QE <desktop-qa-list>
Severity: medium Docs Contact:
Priority: medium    
Version: 7.1CC: aloughla, atragler, bgalvani, lrintel, mleitner, rkhan, thaller, tis, vbenes
Target Milestone: rc   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: 1:1.4.0-0.4.git20160727.9446481f Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-11-03 19:10:08 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Attachments:
Description Flags
[patch] contrib/rpm: improve detection of ppp_version from spec file
none
[patch] dist-git patch for rhel-7.3's NetworkManager.spec none

Description Paul Wouters 2016-05-10 19:04:18 UTC
Description of problem:

%global ppp_version %(rpm -q ppp-devel >/dev/null && rpm -q --qf '%%{version}' ppp-devel || echo -n bad)

this fails in mock because you cannot call rpm inside mock.

NM needs the ppp version number because ppp uses a versioned directory in
/var/lib64/ppp/VERSION/ for its plugins.

It should probably BuildRequire:ppp and look in /var/lib64/ppp/ so it avoids calling rpm.

Comment 1 Tuomo Soini 2016-05-10 19:15:27 UTC
Proper way to do this:

%global ppp_version 2.4.5

BuildRequires: ppp-devel = %{ppp_version}
Requires: ppp = %{ppp_version}

But as long as ppp  package doesn't provide %ppp_version macro you can't automate this. With these changes you at least allow security patching ppp package which is not possible if you require %{version}-%{release}.

So if you want to fix this completely, ppp-devel package could install /etc/rpm/macros.ppp which has version of ppp package so that you can use that macro in NetworkManager package.

Comment 3 Thomas Haller 2016-05-10 19:59:35 UTC
(In reply to Tuomo Soini from comment #1)
> Proper way to do this:
> 
> %global ppp_version 2.4.5
> 
> BuildRequires: ppp-devel = %{ppp_version}
> Requires: ppp = %{ppp_version}


hard-coding it in the spec file isn't awesome either.

I means, you cannot take the SRPM from e.g. Rawhide and build it on Fedora 23 without patching the spec file.


In that case, the detection is better, but maybe it can be improved to also work with mock?

Comment 5 Thomas Haller 2016-07-04 20:24:29 UTC
Created attachment 1176199 [details]
[patch] contrib/rpm: improve detection of ppp_version from spec file

Comment 6 Tuomo Soini 2016-07-04 21:49:53 UTC
Why have that rpm run at all?

Comment 7 Thomas Haller 2016-07-05 07:42:30 UTC
(In reply to Tuomo Soini from comment #6)
> Why have that rpm run at all?

no strong reason. Because it used to work in all situations I encountered so far. So, I added a workaround for other situations (that I did not test myself).
Is there a problem with it? Does it not work?

Comment 8 Tuomo Soini 2016-07-05 13:54:33 UTC
I'd just add following to pppd package:

/etc/rpm/macros.ppp

%ppp_version 2.4.5

and use that in NetworkManager if set. And if not, fail build instead of adding version "bad".

Comment 9 Thomas Haller 2016-07-05 14:21:28 UTC
(In reply to Tuomo Soini from comment #8)
> I'd just add following to pppd package:
> 
> /etc/rpm/macros.ppp
> 
> %ppp_version 2.4.5
> 
> and use that in NetworkManager if set. And if not, fail build instead of
> adding version "bad".

Then you cannot build the NetworkManager SRPM against an older version of pppd (which doesn't provide /etc/rpm/macros.ppp yet). Currently, you can take the SRPM and it will mostly work across RHEL/CentOS/Fedora.
-- ok, apparently not inside mock, but that is what attachment 1178199 [details] tries to fix.

Comment 10 Beniamino Galvani 2016-07-06 09:34:42 UTC
> sed -n 's#.*/\\([0-9][^/]*\\)/$#\\1#p'

I didn't try to build the RPM, but from shell the escaping of backslashes is not needed inside single quote. Is it needed by RPM?

Comment 11 Thomas Haller 2016-07-06 12:39:28 UTC
(In reply to Beniamino Galvani from comment #10)
> > sed -n 's#.*/\\([0-9][^/]*\\)/$#\\1#p'
> 
> I didn't try to build the RPM, but from shell the escaping of backslashes is
> not needed inside single quote. Is it needed by RPM?

It seems so, according to what I tested, the double-escape turns out to be right.

Comment 12 Beniamino Galvani 2016-07-06 13:34:00 UTC
(In reply to Thomas Haller from comment #11)
> (In reply to Beniamino Galvani from comment #10)
> > > sed -n 's#.*/\\([0-9][^/]*\\)/$#\\1#p'
> > 
> > I didn't try to build the RPM, but from shell the escaping of backslashes is
> > not needed inside single quote. Is it needed by RPM?
> 
> It seems so, according to what I tested, the double-escape turns out to be
> right.

Then ok, lgtm.

Comment 13 Thomas Haller 2016-07-07 10:34:59 UTC
(In reply to Thomas Haller from comment #9)
> (In reply to Tuomo Soini from comment #8)
> > I'd just add following to pppd package:
> > 
> > /etc/rpm/macros.ppp
> > 
> > %ppp_version 2.4.5
> > 
> > and use that in NetworkManager if set. And if not, fail build instead of
> > adding version "bad".
> 
> Then you cannot build the NetworkManager SRPM against an older version of
> pppd (which doesn't provide /etc/rpm/macros.ppp yet).

I'm not saying, this isn't the right thing to do. If pppd upstream got fixed now, we might make use of it in a few years (when all distros on which we want to build has it).
There is also an upstream PR for that: https://github.com/paulusmack/ppp/pull/47


Anyway, fixed in our upstream spec file, differently then in the previous patch (after discussion with lrintel) https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=cfd3e1a13c07eb76ea511c1b77f212794ac5adbe

Comment 14 Thomas Haller 2016-07-07 10:36:01 UTC
Created attachment 1177247 [details]
[patch] dist-git patch for rhel-7.3's NetworkManager.spec

Comment 17 errata-xmlrpc 2016-11-03 19:10:08 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHSA-2016-2581.html