Bug 876308

Summary: fedpkg 1.11 breaks parsing of %{!?rhel:0}
Product: [Fedora] Fedora Reporter: Mattias Ellert <mattias.ellert>
Component: fedpkgAssignee: Dennis Gilmore <dennis>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: urgent Docs Contact:
Priority: urgent    
Version: rawhideCC: a.badger, dtardon, ffesti, jreznik, jzeleny, kevin, packaging-team, pknirsch, robatino, tcallawa, tflink
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Linux   
Whiteboard: RejectedBlocker
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-01-18 15:49:14 EST Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Attachments:
Description Flags
This patch implements the proposed changes. none

Description Mattias Ellert 2012-11-13 14:08:20 EST
Description of problem:
The expected interpretation of %{!?rhel:0} when parsed during a fedora build where rhel is not defined is 0:

$ rpm -E '%{!?rhel:0}'
0

However, the fedpkg 1.11 update does something like:
$ rpm --define "rhel %{nil}" -E '%{!?rhel:0}'


I.e. the macro no longer returns the expected value.

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

How reproducible:
Always

Steps to Reproduce:
Build a package whose spec uses the %{!?rhel:0} macro. Example from koji:
https://koji.fedoraproject.org/koji/taskinfo?taskID=4684229
  
Actual results:
Failed build (fedpkg srpm fails)

Expected results:
Successful build
Comment 1 Tom "spot" Callaway 2012-11-14 08:59:46 EST
Grumble grumble grumble. rpm evaluates unset macros differently from macros forced to %{nil}.

Hey Panu, how do you want fedpkg to handle this? What is the "proper" way to undefine a macro at the commandline? 

When we set %{rhel} to 0 (for non-rhel builds), it broke parsing ops.
When we set %{rhel} to %{nil} (for non-rhel builds), it broke different parsing ops.
We can't leave it set (for non-rhel builds), because that is obviously wrong.
Comment 2 Tom "spot" Callaway 2012-11-14 15:39:40 EST
In fact, I'd go so far as to say this is an RPM bug, where RPM treats the result of passing "--define %{rhel} %{nil}" differently from when %{rhel} is unset.
Comment 3 Panu Matilainen 2012-11-16 05:45:38 EST
Macro defined to %{nil} IS different from an undefined one, that's not a bug.
There's no "proper" way to undefine macros from command line ATM (although I think I'll just go ahead and add --undefine cli switch now).

The macro "language" does have %undefine directive though, so there is *a* way to accomplish it, one that should work with pretty much any ancient rpm version as well:

rpm --eval "%undefine rhel" --eval "%{?rel}"
Comment 4 Panu Matilainen 2012-11-16 06:08:46 EST
...or rather, for the reproducer case:

[pmatilai@shell ~]$ rpm --eval '%{rhel}'
6
[pmatilai@shell ~]$ rpm --eval '%undefine rhel' --eval '%{!?rhel:0}'

0
[pmatilai@shell ~]$

Note the extra empty line from the first eval - --eval is not really intended for this kind of use :)
Comment 6 Mattias Ellert 2012-11-19 00:36:41 EST
So the ball is back in fedpkg's court.

fedpkg should use --eval "%undefine rhel" instead of --define "rhel %nil", and similarly for the other macros it wishes to undefine.
Comment 7 Mattias Ellert 2012-11-22 07:31:04 EST
Created attachment 649782 [details]
This patch implements the proposed changes.
Comment 8 Mattias Ellert 2012-11-30 02:55:33 EST
Can I do something more to encourage to fedpkg maintainer to provide a fix for this bag? I have set the priority to urgent, I have set the severity to urgent, I have nominated it as an F18 blocker and I have provided a patch based on the proposed solution.

This is a very disruptive bug. Two thirds of my packages are FTBFS due to it. And I have several package updates that I have checked in to git that I have not been able to build because of it.
Comment 9 Fedora Admin XMLRPC Client 2012-11-30 13:27:42 EST
This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.
Comment 10 Tim Flink 2012-11-30 13:51:28 EST
This may be a disruptive bug but I don't think it qualifies as a blocker for F18. It doesn't violate any of the F18 release criteria and could be fixed with an update post-release.

-1 blocker
Comment 11 Adam Williamson 2012-12-01 03:22:17 EST
-1 blocker, unless there is some consequence of this of which we are not aware. It does not affect the critical functionality of the release package set, so far as I can tell.
Comment 12 Jaroslav Reznik 2012-12-03 09:23:44 EST
-1 blocker
Comment 13 Tim Flink 2012-12-04 15:54:33 EST
I count -3 blocker votes, moving to rejected.
Comment 14 Mattias Ellert 2012-12-06 00:24:22 EST
Hi Dennis!

I hope you have more time to fix things than the previous maintainer seems to have had. Could you please give this issue some priority. This bug is currently making it impossible for me to maintain my packages in Fedora. The solution has been known for three weeks, and a patch is attached to the bug report.
Comment 15 Mattias Ellert 2012-12-07 05:06:09 EST
Hi again!

Can you give an estimate about when you will be able to fix this?

If you prefer I can apply for commit rights to the package and if you are willing to grant them I can provide a package update. Let me know.
Comment 16 Mattias Ellert 2012-12-12 02:42:45 EST
Ping.
Comment 17 Mattias Ellert 2012-12-13 02:52:42 EST
Ping.
Comment 18 Mattias Ellert 2012-12-14 02:50:32 EST
Ping.
Comment 19 Mattias Ellert 2012-12-15 03:36:24 EST
Ping.
Comment 20 Mattias Ellert 2012-12-16 07:15:08 EST
Ping.
Comment 21 Adam Williamson 2012-12-17 16:32:50 EST
Mattias: please stop clogging up people's bug feeds. Daily pings are not polite behaviour.
Comment 22 Mattias Ellert 2012-12-19 04:55:36 EST
(In reply to comment #21)
> Mattias: please stop clogging up people's bug feeds. Daily pings are not
> polite behaviour.

Thank you for your comment. Basically I agree with you. But my frustration with the handling of this bug is just growing every day. This critical regression has prevented me from doing my work as a package maintainer for a long time now. The solution is an almost trivial two-line patch that has been available for more than a month and there has still not been an update to fix it. I understand that not everything can be fixed immediatelly, but it has been a very long time now.

I understand this is not critical to everyone, because not everyone uses these kinds of macros in their specfiles, but for me it is. And it is frustrating feeling neglected and ignored.

I'll hold of the pings for a while. I was hoping to trigger some action from the mantainer, but if it goes on for too long it becomes counterproductive. I did get som feedback offline form the maintainer, but still no update. I am at a loss about what to do if this isn't resolved soon. I would feel a bit silly starting a non-responsive maintainer action against Dennis Gilmore, given that he is one of the core developers in Fedora.
Comment 23 Kevin Fenzi 2012-12-20 17:33:24 EST
Just FYI, Dennis has been traveling of late, and has been very busy trying to get f18 out the door. 

I'll try and see about getting this moving along...
Comment 24 Toshio Ernie Kuratomi 2012-12-21 12:30:49 EST
A quick look doesn't reveal anything that the patch will break internal to fedpkg but using it shows that the command line client now returns extra newlines.  I don't know if anything is depending on parsing the output of fedpkg and will be broken by that so I'm (not a maintainer, just a provenpackager) a little hesitant to apply this.

It seems like the conditionals can be modified to use 0%{!?rhel} instead of %{!?rhel:0} to workaround this.
Comment 25 Mattias Ellert 2012-12-27 17:37:56 EST
I know the 0%{!?rhel} hack can be used. Personally, I find it very confusing and not very accurately expressing what the conditional is meant to express.

I do really hope that noone is suggesting that this bug should not be fixed. The %{!?rhel:0} macro has worked in specfiles for years and only a badly implemented change in the latest version of fedpkg broke it. The choice between fixing two buggy lines in fedpkg and forcing the implementation of a workaround in hundreds of lines of perfectly valid specfiles to be rather obvious in favour of fixing fedpkg.
Comment 26 Tom "spot" Callaway 2013-01-07 15:12:49 EST
If the --eval workaround didn't emit an extra newline, I'd apply it without concern. Really, using --undefine is what we want here, but the problem is that it wouldn't work on RHEL era RPM (or even-non-rawhide Fedora). *sigh*

It might be possible to have pyrpkg (srpm is "rpkg") process the output (I'm guessing the newline goes to stdout) and strip out empty lines (just newline), but I don't think that is right either.

I think we will just have to live with the newline here. That seems less annoying than an entire class of macros not evaluating. Barring any opposition, I'll commit the eval fix and rebuild tomorrow.
Comment 27 Fedora Update System 2013-01-08 13:17:31 EST
fedpkg-1.11-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/fedpkg-1.11-2.fc18
Comment 28 Fedora Update System 2013-01-08 13:17:46 EST
fedpkg-1.11-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/fedpkg-1.11-2.fc17
Comment 29 Mattias Ellert 2013-01-09 00:56:56 EST
Thanks for creating the updates. The rawhide buildroot is now working again. Would it be possible to add a buildroot override for the f18 update in order to have a working f18 buildroot while we wait for the update to reach stable.
Comment 30 Fedora Update System 2013-01-09 04:02:17 EST
Package fedpkg-1.11-2.fc17:
* should fix your issue,
* was pushed to the Fedora 17 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing fedpkg-1.11-2.fc17'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-0504/fedpkg-1.11-2.fc17
then log in and leave karma (feedback).
Comment 31 Fedora Update System 2013-01-18 15:49:16 EST
fedpkg-1.11-2.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 32 Fedora Update System 2013-01-19 22:03:14 EST
fedpkg-1.11-2.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.