Bug 950282

Summary: NetworkManager-pptp spec file
Product: [Fedora] Fedora Reporter: Dan Fruehauf <malkodan>
Component: NetworkManager-pptpAssignee: Dan Williams <dcbw>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: awilliam, dcbw, dennis, malkodan
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
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: ---
Bug Depends On: 949132    
Bug Blocks: 913825    
Attachments:
Description Flags
Patch to fix spec file
none
Patch to fix spec file none

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.