Bug 1820349

Summary: rpmbuild rpm >= 4.15 source/patch handling regression?
Product: [Fedora] Fedora Reporter: Nicolas Mailhot <nicolas.mailhot>
Component: rpmAssignee: Packaging Maintenance Team <packaging-team-maint>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: high Docs Contact:
Priority: unspecified    
Version: rawhideCC: fedoraproject, igor.raits, mjw, packaging-team-maint, petersen, pmatilai, pmoravco, vmukhame
Target Milestone: ---Keywords: Reopened
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2020-05-04 08:58:53 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 1827964    

Description Nicolas Mailhot 2020-04-02 19:52:16 UTC
Description of problem:

My private build box has
%_sourcedir %{_topdir}/SOURCES/%{name}

in /home/nim/.rpmmacros (to deal with a huge number of package sources)

I’ve now updated it to the F33 rpm 4.16 prerelease to prepare making use of rpm 4.16 feature

To my dismay, spec files like
https://src.fedoraproject.org/rpms/sil-mondulkiri-extra-fonts/blob/f32/f/sil-mondulkiri-extra-fonts.spec

That were processed fine by rpmbuild less than a month ago, for the F32 release, no longer work

$ rpmbuild -bs /tmp/sil-mondulkiri-extra-fonts.spec 
error: Bad source: /var/lib/builder/rpmbuild/SOURCES/%{name}/Mondulkiri-5.300.zip: No such file or directory

It seems rpmbuild fails to evaluate %{name} completely before looking for source files

Version-Release number of selected component (if applicable):
rpm-4.15.90-0.git14971.2.fc33

Comment 1 Nicolas Mailhot 2020-04-02 19:54:06 UTC
(filing there because I hit it Fedora-side, do tell me if you prefer this kind of thing to be filled upstream)

Comment 2 Nicolas Mailhot 2020-04-02 20:06:30 UTC
More testing

$ rpm -q --qf "%{name}\n" --specfile sil-mondulkiri-extra-fonts.spec 
sil-mondulkiri-extra-fonts
sil-ratanakiri-fonts
sil-oureang-fonts
sil-busra-moe-fonts
sil-busra-dict-fonts
sil-busra-high-fonts
sil-busra-bunong-fonts
sil-busra-xspace-fonts
sil-busra-diagnostic-fonts
sil-busra-dot-fonts
sil-mondulkiri-extra-fonts-all
sil-mondulkiri-extra-fonts-doc

→ direct evaluation work, except that’s the subpackage list, not the srpm name, useless in %_sourcedir

Let’s checj if rpm understands %{source_name} like dnf

rpm -q --qf "%{source_name}\n" --specfile sil-mondulkiri-extra-fonts.spec 
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"
error: incorrect format: unknown tag: "source_name"

Comment 3 Nicolas Mailhot 2020-04-02 20:10:22 UTC
It seems to break mock builds too

start: build setup for dejavu-fonts-2.37-11.0.fc33.src.rpm

error: Bad source: /builddir/build/SOURCES/%{name}-urn-dtd.patch: No such file or directory

Comment 4 Nicolas Mailhot 2020-04-02 20:14:28 UTC
(I workarounded this last one by moving the Patch declaration between Name: and Summary:)

Comment 5 Panu Matilainen 2020-04-03 06:38:27 UTC
It'd seem only natural to me that you can only refer to %{name} after actually defining it, I'm mostly puzzled how that could have ever worked.

I was about to say I can't begin to imagine what would've caused such a behavior change in 4.16 but indeed, this behavior actually changed between 4.14 and 4.15. I'll chase it down to understand what's going on, but really, a value needs to be defined before it can be used.

Comment 6 Panu Matilainen 2020-04-03 06:55:31 UTC
https://github.com/rpm-software-management/rpm/commit/93604e2c3b8ed8a2b1ac77c4c43856e68b395c49 is where the behavior changed, so indeed in 4.15 already, changing summary accordingly.

As the message says "no functional changes" it does deserve a closer look.

Comment 7 Panu Matilainen 2020-04-03 08:44:24 UTC
Opened an upstream ticket to discuss the case: https://github.com/rpm-software-management/rpm/issues/1161

Comment 8 Nicolas Mailhot 2020-04-03 08:58:15 UTC
Please note that the dejavu breakage, while clearly related, is not exactly the same as the one in
https://src.fedoraproject.org/rpms/sil-mondulkiri-extra-fonts/blob/f32/f/sil-mondulkiri-extra-fonts.spec

That one is worse. I can workaround dejavu, I can not workaround the other.

And %{name} is an extremely pervasive construct, reused everywhere in most specs. You can both have Name: defined from other macros, and other macros defined from %{name}. It is most illusory to think it would be possible to impose a specific declaration ordering, any ordering will break a huge number of specs.

Name and other srpm-level properties are more global than %global

Comment 9 Panu Matilainen 2020-04-03 09:06:15 UTC
sil-mondulkiri-extra-fonts.spec should work fine if you move Name before other constructs referring to it. The name is not constructed from any prior macros at all, so don't claim you can't do that. I don't see that as a "workaround" at all.

Comment 10 Panu Matilainen 2020-04-03 09:09:54 UTC
Oh and FWIW, I'm fully aware that there are exist complicated constructs forming name, version and release from macros defined before them. That's absolutely fine. My objection here is that you expect to refer to a value that hasn't been declared yet, and sources/patches is the only special case that ever allowed that at all (more or less accidentally I'd claim)

Comment 11 Panu Matilainen 2020-04-03 09:14:38 UTC
Oh, actually tried it out: yes sil-mondulkiri-extra-fonts.spec works just fine if Name is moved towards the start where people usually put it (traditionally before Version but that's a mere convention)

Comment 12 Nicolas Mailhot 2020-04-03 09:26:43 UTC
Just Name or the whole block? IIRC I had to put it there because rpm kept trying to make things declared after %description part of the package description.

I am well aware that a lot of the rpm design is accidental;) But not all the accidental elements are bad, given they passed the test of time.

Comment 13 Panu Matilainen 2020-04-03 09:38:03 UTC
Just the Name tag, as that's what defines %{name}, literally.

Comment 14 Nicolas Mailhot 2020-04-04 10:53:55 UTC
Ok, I tested once again, and it is still broken.

To make things clean and templateable and documentable (I do write a lot of packaging guidelines) Source declarations must occur before package header declarations or after them, not in magic middle places.

If I put them before, things break because of %name

If I put them bellow, things break because of %description (that was reported 7 years ago as #564613 however the "fix" was not deemed worthy to be pushed to rawhide and as a result, when I tested it two years after I had forgotten the Source part)

SourceX then (sub)package headers →
  error: Bad source: /var/lib/builder/rpmbuild/SOURCES/%{name}/dejavu-fonts-version_2_37.tar.gz: No such file or directory

  %{name} not evaluated

(sub)package headers then SourceX →
  error: No source number 0

  %descriptions eat the Source declaration

(sub)package headers then %end then SourceX (issue #564613 "fix") → 
  error: line 149 doesn't belong to any section: Source0:  https://github.com/dejavu-fonts/dejavu-fonts/archive/version_2_37/dejavu-fonts-version_2_37.tar.gz

  Source declaration is ignored
  (the fix request was to create an end of description marker, that was transformed into creating an end of section marker, serves us right for papering over the root cause with a different fix I guess)

And before you say “The Name block is special and different from the other %package -n blocks”. Functionaly it is strictly identical. And, is is indeed generated the same way in Fedora specs 

  name = sub and "%package     -n " or "Name:           "
  print(
    name ..
    rpm.expand(
      "%{currentfontpkgname}\n" ..
      "Summary:        %{currentfontsummary}\n" ..
      "License:        %{currentfontlicense}\n" ..
      "BuildArch:      noarch\n" ..
      "BuildRequires:  fonts-rpm-macros\n" ..
      "Requires:       fontpackages-filesystem\n" ..
      "%{?currentfontpkgheader}\n" ..
      "%description -n %{currentfontpkgname}\n") ..
    fedora.wordwrap("%{?currentfontdescription}") ..
    "\n")


No room to inject magic Source blocks in the middle here

Comment 15 Nicolas Mailhot 2020-04-04 11:09:39 UTC
If I may add: pretty much all of those specs already put the description text in a variable via expand (this is a requirement to reuse the description block in generated appstream files for examples. So all the misery here, is because:

%global mytext %{expand:
At vero eos et accusamus et iusto odio dignissimos ducimus qui blanditiis 
praesentium voluptatum deleniti atque corrupti quos dolores et quas molestias
excepturi sint occaecati cupiditate non provident, similique sunt in culpa qui 
officia deserunt mollitia animi, id est laborum et dolorum fuga.

Et harum quidem rerum facilis est et expedita distinctio. Nam libero tempore, 
cum solutanobis est eligendi optio cumque nihil impedit quo minus id quod
maxime placeat facere possimus, omnis voluptas assumenda est, omnis dolor
repellendus.

Temporibus autem quibusdam et aut officiis debitis aut rerum necessitatibus
saepe eveniet ut et voluptates repudiandae sint et molestiae non recusandae.
Itaque earum rerum hic tenetur a sapiente delectus, ut aut reiciendis
voluptatibus maiores alias consequatur aut perferendis doloribus asperiores
repellat.}

… which is a nicely contained %{mytext} block, needs to be transformed into

%description
%{mytext}

That is unbounded and eats the following spec lines.

Comment 16 Panu Matilainen 2020-04-06 08:25:08 UTC
Sorry but I cannot make heads or tails out of that, I cannot possibly know what exactly you tried there. 
I didn't suggest moving Name to somemumble in the middle but to early in the spec like everybody else does. 
Both the aforementioned font packages seem to build just fine if I move the Name just before Version. If there's something wrong with the resulting packages because of that, I wouldn't know because I'm not familiar with these packages.

If you want me to help, provide full reproducer cases with full srpm/spec and commands how to verify. I'm not trying to be difficult but to get anywhere, we need to be talking about the same thing. And please try to avoid mixing multiple issues - if %end isn't working then open another bug for that.

Comment 17 Nicolas Mailhot 2020-04-06 11:10:55 UTC
I’m investigating, it is messy yes. Years of spec dev and factorisation, and refactorisation.



First, %end works as defined upstream: it’s a section end. What I hadn’t understood then is that description is not a section. So it works to close description (the initial need), but only when %description is the last part of a section, as a side effect of closing the section. It does not work when you need to put more things in the section after description. 

So, writing

%description 

Some text

%end

SourceX: …

does not work

(sub)package headers then %end then SourceX (issue #564613 "fix") → 
  error: line 149 doesn't belong to any section: Source0:  https://github.com/dejavu-fonts/dejavu-fonts/archive/version_2_37/dejavu-fonts-version_2_37.tar.gz




Second, what my packages have been doing (successfully, before this change that is)

<generic SRPM stuff, including sources>

<headers for produced package 1: Name, Summary, description…>
<headers for produced package 2: Name, Summary, description…>
<headers for produced package 3: Name, Summary, description…>

When you ask "just put Source declaration after Name:" that means put them after one or several of those blocks, and the %description in the final block eats the Source declarations. Fine tuning (putting source after Name, but before description) only "works" for manual headers, and would be a PITA to document (because that makes manual declarations different from automated ones).



You can see it in:

https://src.fedoraproject.org/rpms/sil-mondulkiri-fonts/blob/f32/f/sil-mondulkiri-fonts.spec

and

https://src.fedoraproject.org/rpms/sil-mondulkiri-extra-fonts/blob/f32/f/sil-mondulkiri-extra-fonts.spec

(interesting specs because they’re the same project upstream, published in two parts, one named after the main font family, the other not. So they only differ on the way the SRPM is named.)


Both `rpmspec -P` to the exact same declaration order. It is important, maintenance-wise, that they keep `rpmspec -P`-ing to the same declaration order, to have a *single* pattern to maintain without special cases.




Practically, moving Sources down while keeping the same declaration order means moving them at least after line 36 in sil-mondulki-fonts:

https://src.fedoraproject.org/rpms/sil-mondulkiri-fonts/blob/f32/f/sil-mondulkiri-fonts.spec#_36

because that’s the line that generates Name: (and a lot of other stuff besides).
Though, to be future-proof, it would probably need to move just before line 47 (%prep), because I can't assume the %doc subpackage will keep getting generated manually.

https://src.fedoraproject.org/rpms/sil-mondulkiri-fonts/blob/f32/f/sil-mondulkiri-fonts.spec#_47



The mirror change, in sil-mondulkiri-extra-fonts, would be moving Sources after line 164

https://src.fedoraproject.org/rpms/sil-mondulkiri-extra-fonts/blob/f32/f/sil-mondulkiri-extra-fonts.spec#_164

or, to be future-proof, just before %prep
https://src.fedoraproject.org/rpms/sil-mondulkiri-extra-fonts/blob/f32/f/sil-mondulkiri-extra-fonts.spec#_175


All those changes fail because Sources kept getting eaten by the preceding %descriptions

1. option 1: moving after the %fontpkg -a call:

$ rpmbuild -bs ~/fedora/fedpkg/sil-mondulkiri-fonts/sil-mondulkiri-fonts.spec
error: No source number 0

$ LANG=C.UTF8 rpmbuild -bs  ~/fedora/fedpkg/sil-mondulkiri-extra-fonts/sil-mondulkiri-extra-fonts.spec 
error: No source number 0

2. option 2: moving before %prep:

$ rpmbuild -bs ~/fedora/fedpkg/sil-mondulkiri-fonts/sil-mondulkiri-fonts.spec
error: No source number 0

$ LANG=C.UTF8 rpmbuild -bs  ~/fedora/fedpkg/sil-mondulkiri-extra-fonts/sil-mondulkiri-extra-fonts.spec 
error: No source number 0


I should stress that both of those specs are representative of how we package a lot of fonts in Fedora today. They’re only “special” because they are almost identical, while showcasing two patterns (one where the first subpackage is named like the SRPM, the other where it is different).


I’m pretty sure it will also affect the 1000 or so Go packages in Fedora, since their macros use the same principles as the fonts one (fonts are easier to test and understand, they don’t have the huge Go buildchain complexity)

Comment 18 Nicolas Mailhot 2020-04-06 11:19:27 UTC
However, as I wrote in github, while fixing %description closing properly would be nice, and enable using some form of “natural” programming declaration order, I don’t understand what is the pressing need to evaluate SourceX value at the point it occurs in the spec, and not at the end of the section, before %prep, when all the other stuff it may reference has been properly set up by the packager.

It’s not as if the SRPM can be assembled before all the Sources have been declared, and they can keep being declared till the %prep line as far as I understand things.

Comment 19 Nicolas Mailhot 2020-04-06 11:20:09 UTC
(PatchX is the same BTW)

Comment 20 Nicolas Mailhot 2020-04-06 12:33:45 UTC
Just to be clear: I’m probably able to kludge my own specs so they work (sort of). This is not about that. The change breaks *two* official packaging patterns in Fedora (as documented in our guidelines).

And, again, I am willing to update the Fedora guidelines, and Fedora macros, if that makes things better and cleaner (not enthusiastic mind you, it’s a lot of work, and people are probably going to request one if not two change pages).

But anything in guidelines needs to be more regular clean and general and explainable than "move this line here in this spec".

Right now the only way I see to change specs while keeping them regular and packager-friendly is to move Sources/Patches just before prep with a one-liner barrier to force closing description in the case the preceding description has not been generated by a macro (generated stuff can be changed to add the barrier by default). Which will still make quite a lot of people unhappy, because it creates F33+-only specs, unless the barrier is made available in previous releases too.

Still, I'm would be ready to sacrifice myself to the lynching mob, if it was a clean future-proof change. Right now it isn’t. It’s replacing one magic rpm parser side-effect quirk with another magic side-effect quirk. Side-effect that makes spec files *less* regular, not *more* regular.

At least “Sources and Patches are last, before %prep, when all variables and Tags that initiate variables have been declared” is clean explainable and not magic ordering. Except, it does not work today.

Comment 21 Panu Matilainen 2020-04-07 06:36:21 UTC
Way too much information. I see three examples mentioned:

1) dejavu-fonts
2) sil-mondulkiri-fonts
3) sil-mondulkiri-extra-fonts

In 1) and 3), Name is manually specified in towards the end of spec preamble, and with these I can reproduce the issue of sources not found if %{name} is part of %_sourcedir. If there's an answer to "what breaks if you do so", it's lost in the noise for me.

2) builds without any errors for me whether %{name} is part of %_sourcedir or not, so I don't know what I'm supposed to be looking at.

Again: simple concrete reproducers, please. If I need additional context I'll ask for it. Thanks.

P.S. Thinking about this did give me an idea that would also clean up some other kludgery in rpmbuild, will investigate.

Comment 22 Nicolas Mailhot 2020-04-07 06:58:09 UTC
1. You asked to simplify things, forget about dejavu, sil-mondulkiri-fonts and sil-mondulkiri-extra-fonts are representative of the two main patterns used by the 1000+ Go and Fonts Fedora spec files.

2. Please find a solution that works for both those specs (ie they pass rpmbuild as before and rpmspec -P produces the same declaration order so there are no automation side effects baked in). I'll generalise to the rest of the spec pool if necessary

I already described line by line what I attempted in those spec and how it did not work. Just try it. You need the git checkout anyway to get the rest of the things those specs need for building (and I can not give it to you as a srpm because, that does not work anymore)

Comment 23 Panu Matilainen 2020-04-07 13:39:35 UTC
To step back a bit:

The order of tags is not an issue per se. It's the use of undefined macros.
 
Going back to the dejavu-fonts example, you don't need to move Name anywhere at all, just don't use %{name} before it. As in, call the patch by its actual name instead of using %{name} in it. 

That package was building just fine with both rpm 4.15 and 4.16 until commit 03ba0905119e48fd3e822411f92496c2fb43d8f0 added "%{name}-urn-dtd.patch", after which it no longer builds with neither 4.15 or 4.16. In fact it already was once successfully built with 4.16 alpha, just before that patch was added:
https://kojipkgs.fedoraproject.org//packages/dejavu-fonts/2.37/8.fc33/data/logs/noarch/root.log

It doesn't appear that there's any kind of mass-breakage in place, this stuff has been built over and over with rpm 4.15 which behaves the same as 4.16 wrt this.

Comment 24 Nicolas Mailhot 2020-04-07 14:12:26 UTC
For historical reasons dejavu is not representative of the current state of our guidelines and has quite a lot of one-of-a-kind things inside

sil-mondulkiri-fonts and sil-mondulkiri-extra-fonts are better cleaner and simpler examples

Please propose an ordering that works for both of them.

Comment 25 Panu Matilainen 2020-04-08 11:25:21 UTC
Both those specs work just fine in a setup that doesn't use spec-defined macros in %{_sourcedir}. 

As an example: https://koji.fedoraproject.org/koji/taskinfo?taskID=43124256

Comment 26 Panu Matilainen 2020-04-20 11:21:18 UTC
The behavior change now documented in https://rpm.org/wiki/Releases/4.15.0  and rpm >= 4.15.90-0.git14971.6 will issue a warning if unexpanded macros are detected in build tree setup such as %_topdir, %_sourcedir etc. The warning will be backported to 4.15 in the next maintenance release.

Comment 27 Nicolas Mailhot 2020-04-26 00:55:53 UTC
Adding a broken warning after breaking things is not fixing anything.

Proposing “resolutions” without implementing them not merging the result once you’ve forced others to do your clean up is not fixing things either

Comment 28 Panu Matilainen 2020-04-27 06:54:07 UTC
The point of the warning is to tell you that you're using an unsupported configuration. 

If you want us to reconsider then you need to present new evidence. Point us to the mass breakage that must be evident in koji if there is one - there have been two Fedora mass rebuilds with this change in place. 

I'm also willing to entertain the possibility of there being something else going on *in addition*, but going around in circles chanting "we're doomed" and personal insults is not helpful. You need to point us to build failures under supported configuration.

Comment 29 Panu Matilainen 2020-04-28 13:04:09 UTC
> I’m pretty sure it will also affect the 1000 or so Go packages in Fedora, since their macros use the same principles as the fonts one (fonts are easier to test and understand, they don’t have the huge Go buildchain complexity)

So I just looked at a dozen or so golang packages. Turns out they were already using a scheme much like I suggested:

---
%global goipath github/some/path
Version: x.y.z

%gometa

Name:           %{goname}
[...]
URL:            %{gourl}
Source0:        %{gosource}
---

...so they never were affected by any of this at all. Which goes to explain the lack of bug reports and build failures that we've been asking for to support the claimed mass breakage. All this hot water and wasted time and energy for a non-issue. I'm not amused.

Comment 30 Panu Matilainen 2020-05-04 08:58:53 UTC
No new evidence of mass breakage, nothing to reconsider.

All this wolf-crying and other language used in the related escalations certainly made me lose any appetite at looking at the more fundamental issues (as I hinted in comment #21). Bye.