GNOME Bugzilla – Bug 736954
RPM won't build
Last modified: 2014-10-03 15:18:27 UTC
There's essentially two different things to review here. One fixes a build on Rawhide (where ppp is too new), while other one fixes a fallback from libnm-vpn eviction: commit 8538bcedace602d5b9f42fe57dd22c4999a740d3 Author: Lubomir Rintel <lkundrak@v3.sk> Date: Fri Sep 19 10:41:16 2014 +0200 contrib/rpm: libnm-vpn is no more Removed in: [280b1e506] libnm: merge libnm-vpn into libnm commit 56665ae1829bb270545b675c20f08648ac58e72d Author: Lubomir Rintel <lkundrak@v3.sk> Date: Sun Sep 7 23:36:35 2014 +0200 contrib/rpm: Don't hardwire a single ppp version Build against whatever is actually present.
Created attachment 286602 [details] [review] contrib/rpm: Don't hardwire a single ppp version
Created attachment 286603 [details] [review] contrib/rpm: libnm-vpn is no more
Comment on attachment 286603 [details] [review] contrib/rpm: libnm-vpn is no more Attachment 286603 [details] pushed as b769c05 - contrib/rpm: libnm-vpn is no more
Comment on attachment 286602 [details] [review] contrib/rpm: Don't hardwire a single ppp version >+%define ppp_version %(pppd --help 2>&1 |awk 'BEGIN {v="bad"} /pppd version/ {v=$NF} END {print v}') I think we want to keep this more-or-less the same as the actual fedora spec file, and using the package itself to figure out what version of the package to pull in clearly won't work on the build machines. I'd say just add another version clause to the %if. (Or maybe we can just switch to ">=" rather than "="?)
Also, maybe we should make it so that "make check" (or at least "make distcheck") tries to do an RPM build on Fedora? This isn't the first time I've forgotten to fix the spec file after making build changes...
(In reply to comment #4) > (From update of attachment 286602 [details] [review]) > >+%define ppp_version %(pppd --help 2>&1 |awk 'BEGIN {v="bad"} /pppd version/ {v=$NF} END {print v}') > > I think we want to keep this more-or-less the same as the actual fedora spec > file, and using the package itself to figure out what version of the package to > pull in clearly won't work on the build machines. I'd say just add another > version clause to the %if. (Or maybe we can just switch to ">=" rather than > "="?) That doesn't work because we need the ppp version for the pppd-plugin-path. So we really do either need to auto-detect the version, or hardcode it somewhere in the specfile. Random idea: build a small configure-time program that #includes pppd/patchlevel.h and uses VERSION to print out the pppd version and use that instead of having to specify it?
(In reply to comment #4) > (From update of attachment 286602 [details] [review]) > >+%define ppp_version %(pppd --help 2>&1 |awk 'BEGIN {v="bad"} /pppd version/ {v=$NF} END {print v}') > > I think we want to keep this more-or-less the same as the actual fedora spec > file, Yep, I'd update this in the Fedora spec as soon as it would pass review. > and using the package itself to figure out what version of the package to > pull in clearly won't work on the build machines. I'd say just add another > version clause to the %if. (Or maybe we can just switch to ">=" rather than > "="?) It would work. I removed the version macro from the BR, therefore the build time dependency would be satisfied with any version available, and the macro would only expand for the runtime Requires. (In reply to comment #5) > Also, maybe we should make it so that "make check" (or at least "make > distcheck") tries to do an RPM build on Fedora? This isn't the first time I've > forgotten to fix the spec file after making build changes... Well, the SPEC runs make check... distcheck might be a good idea though. (In reply to comment #6) ... > Random idea: build a small configure-time program that #includes > pppd/patchlevel.h and uses VERSION to print out the pppd version and use that > instead of having to specify it? We'd still need the version in SPEC though, to generate the Require. Requested addition of a pkg-config file to fix this in the long run: https://github.com/paulusmack/ppp/pull/22
I would prefer some hard-coded %if-%else preamble... %define ppp_version 2.4.5 %if (0%{?fedora} %if 0%{?fedora} == 20) %define ppp_version 2.4.6 %else %define ppp_version 2.4.7 %endif %endif I prefer that, because the spec file should produce a similar result to the official packages. And the official spec files should specify the version explicitly, not detecting it.
(In reply to comment #8) > And the official spec files should specify the version explicitly, not detecting it. What would be the point of doing that?
The point would be that as a distribution is something relatively static, also the SPEC file explicitly names the version -- for each distri/release. Anyway. I applied the patch as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=53b9504544cbfabb74b97b32ab52f14c7d202d93
Review of attachment 286602 [details] [review]: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=53b9504544cbfabb74b97b32ab52f14c7d202d93
Thank you for applying it, as well as subsequent fix.