Bug 1820349
Summary: | rpmbuild rpm >= 4.15 source/patch handling regression? | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Nicolas Mailhot <nicolas.mailhot> |
Component: | rpm | Assignee: | Packaging Maintenance Team <packaging-team-maint> |
Status: | CLOSED WONTFIX | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | high | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | 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
(filing there because I hit it Fedora-side, do tell me if you prefer this kind of thing to be filled upstream) 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" 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 (I workarounded this last one by moving the Patch declaration between Name: and Summary:) 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. 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. Opened an upstream ticket to discuss the case: https://github.com/rpm-software-management/rpm/issues/1161 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 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. 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) 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) 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. Just the Name tag, as that's what defines %{name}, literally. 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 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. 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. 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) 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. (PatchX is the same BTW) 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. 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. 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) 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. 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. 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 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. 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 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. > 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.
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. |