Bug 950282 - NetworkManager-pptp spec file
NetworkManager-pptp spec file
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: NetworkManager-pptp (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Dan Williams
Fedora Extras Quality Assurance
:
: 913860 (view as bug list)
Depends On: 949132
Blocks: F19FTBFS
  Show dependency treegraph
 
Reported: 2013-04-09 22:10 EDT by Dan Fruehauf
Modified: 2013-07-15 21:43 EDT (History)
4 users (show)

See Also:
Fixed In Version: NetworkManager-pptp-0.9.8.2-2.fc19
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-07-15 21:43:59 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Patch to fix spec file (5.83 KB, patch)
2013-04-09 22:10 EDT, Dan Fruehauf
no flags Details | Diff
Patch to fix spec file (5.84 KB, patch)
2013-04-11 08:34 EDT, Dan Fruehauf
no flags Details | Diff

  None (edit)
Description Dan Fruehauf 2013-04-09 22:10:23 EDT
Created attachment 733441 [details]
Patch to fix spec file

Description of problem:
NetworkManager-pptp spec file does not conform to many Fedora Packaging guidelines

Version-Release number of selected component (if applicable):


How reproducible:
Look at spec file

Steps to Reproduce:
1. Includes for instance '%define' instead of '%global' and more
  
Actual results:
I'm providing a patch which brings NetworkManager-pptp.spec to a better state and also splits it into NetworkManager-pptp and NetworkManager-pptp-gnome

Expected results:


Additional info:
I've gone through the process of changing NetworkManager-ssh.spec (based on NetworkManager-openvpn.spec) to conform with Fedora packaging standards and decided it'll be a good idea to implement these changes for the rest of the NetworkManager-* VPN plugins out there.
Comment 1 Dan Fruehauf 2013-04-11 08:34:19 EDT
Created attachment 734156 [details]
Patch to fix spec file
Comment 2 Dan Fruehauf 2013-04-11 08:36:38 EDT
Note about patch:
Contains a deliberate error (TODO) and needs to be replaced with latest known version, because of the package split. As Dan (Williams) mentioned, it is mandatory for updates to work correctly.
Comment 3 Adam Williamson 2013-05-10 16:55:19 EDT
Came across this while looking for packages that do scriptlets wrong. Looks good, but a few comments:

1. TODO Obsoletes: NetworkManager-pptp = REPLACE_WITH_LATEST_KNOWN_VERSION

Why? That doesn't appear to make any sense at all. Why would you want NetworkManager-pptp to obsolete...NetworkManager-pptp ?

2. Since upstream claims that disabling the .desktop file and icon is temporary - the stuff in Makefile.am is commented out with the note "uncomment when nmce gets --import support" - wouldn't it be better to comment out rather than remove the bits that are related to that (the Post and PostUn requirements, the scriptlets themselves, and the filenames)? Or are these now never planned to come back (in which case upstream should be adjusted)?

3. While the spec files are being revamped, what about https://bugzilla.redhat.com/show_bug.cgi?id=811930 ? stefw suggests that all these NM plugins should depend on libgnome-keyring, not gnome-keyring.

Thanks!
Comment 4 Adam Williamson 2013-05-10 17:12:14 EDT
Ah. dcbw explains the idea is that having Obsoletes statements in both the new -gnome subpackage and the main package will ensure that both are installed on updates.

I think that, since NetworkManager-pptp-gnome will require NetworkManager-pptp, you only need an Obsoletes: statement in NetworkManager-pptp-gnome for things to work. And I think it should be something like:

Obsoletes: NetworkManager-pptp < 1:0.9.3.997-5

rather than using a = (if you use = it will only obsolete one specific version, so won't that break if someone happens to be updating from something older?)

It's quite hard to use = in Obsoletes statements in general because you have to get the whole NEVR precisely correct, so I picked up the trick of just finding the highest build you want to obsolete, adding 1 to the revision, and using <.

I just play a packaging expert on TV, though, so you might want to run this by Spot or Toshio or someone.
Comment 5 Dan Fruehauf 2013-05-10 21:23:40 EDT
Hey Adam,

Thanks for your comments. I an definitely not an expert and this ticket was opened as a suggestion in order to conform better with packaging standards. I'm still on the "steep learning curve" side of things...

1. The 'Obsoletes:' line came after dcbw suggested me to add it. I understand your concern in your second comment however I think I'm not the best person to comment as I am not very familiar with upgrading and obsoleting...

2. Can comment it if you insist, but I personally don't like unused commented clutter. Comments are comments, not features for future integration of who knows what. It'll be in the git history should we ever require to add it again.

3. Makes sense. Can definitely change it if we're sure about it. Perhaps also make the other bug depending on this one?

Happy to prepare new patches in all of these tickets once these issues are resolved.

Cheers!
Comment 6 Adam Williamson 2013-05-10 23:29:23 EDT
Hi Dan!

1: it may be best if we just run this by someone on the packaging committee as a tie breaker I guess :) I think I'm right, but I don't want to tell you guys to do something that turns out to be wrong...

2: oh, I don't insist on anything, I'm just sticking my nose in :) Usually I like clean specs too, I hate when people comment out dropped patches and stuff, but I do like commenting out stuff that's explicitly _temporarily_ disabled because it acts as a reminder to put it back in. If you have the .desktop-related things in there but commented out, then as soon as you do a build which fails because upstream activated that stuff again, you'll know "hey, I should re-enable all these other bits too". The git history is a bit too 'hidden' for that purpose, it's not looking you in the face all the time. Just my opinion, though, like I said. It may be worth clarifying with dcbw whether he's still actually planning to turn that stuff back on upstream, or whether it's kind of dead now.

3: Sure, we could do that - again I'd probably check with dcbw if he thinks stefw is correct, if there's not some special reason for the dep, but it looks like it might as well be changed.

thanks for taking my thoughts into consideration!
Comment 7 Fedora Update System 2013-07-11 06:54:30 EDT
NetworkManager-pptp-0.9.8.2-2.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/NetworkManager-pptp-0.9.8.2-2.fc19
Comment 8 Fedora Update System 2013-07-11 22:59:13 EDT
Package NetworkManager-pptp-0.9.8.2-2.fc19:
* should fix your issue,
* was pushed to the Fedora 19 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing NetworkManager-pptp-0.9.8.2-2.fc19'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-12769/NetworkManager-pptp-0.9.8.2-2.fc19
then log in and leave karma (feedback).
Comment 9 Jirka Klimes 2013-07-15 11:11:00 EDT
*** Bug 913860 has been marked as a duplicate of this bug. ***
Comment 10 Fedora Update System 2013-07-15 21:43:59 EDT
NetworkManager-pptp-0.9.8.2-2.fc19 has been pushed to the Fedora 19 stable repository.  If problems still persist, please make note of it in this bug report.

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