Dynamic Spec generation #1485
Dynamic Spec generation #1485
Conversation
|
cc @davide125 |
|
Didn't look with a microscope yet (some whitespace and spelling issues there at least) but on the whole this looks really neat set of patches. Which is usually a good sign 👍 Do add some test-case(s) though, no matter how artificial. |
|
A couple of random thoughts:
|
|
More random thoughts as they trickle through this old-but-not-obsolete (yet) processor of mine... 🧠
|
|
cc: @dcermak |
|
cc: @Vogtinator |
|
This looks intriguing! Do I understand it correctly that you'd essentially do a |
Is there a reason we can't just push it back into |
Basically yes. As this is run in the %build or %install section you would need to modify your spec generator to not output these. Long term this may also be used to generate the debuginfo sub packages. But as they are more closely coupled to the rpmbuild internals there are some more pieces needed to make this feasible. |
Yes, we could. I have thinking about this quite a lot. But this is much more complicated and harder to use. Generating a piece of spec file is easy and can be done from any programming language. This is a huge benefit if you want to do language specific solutions as you have easy access to all the domain specific tooling. But there are a few use cases that will benefit from direct access to the rpmbuild internals. Debuginfo being one of them. Otoh I dislike the fact that the internal state of rpmbuild getting altered without an artefact that documents that. Yes, this already is the case for debuginfo packages. But I would rather have less of that than more. |
|
It would be nice to see some simple example(s) :-) |
build/parseSpec.c
Outdated
| int argc = 0; | ||
| int i; | ||
|
|
||
| char * specPattern = rpmGenPath("%{u2p:%{_builddir}}", spec->buildSubdir, "__rpmbuild_*.specpart"); |
There was a problem hiding this comment.
__rpmbuild_*.specpart still looks quite the eyesore to me. These are not rpm internal things, but files we expect others to produce. I'd think *.specpart would be enough to differentiate from other stuff.
Also there's still the option of creating a special directory (.rpmbuild or such in the builddir) which could be used for such things, including but not limited to debuginfo filelists, to avoid cluttering the main build directory (that gets requested separately every now and then). Such a thing doesn't have to be part of this PR, but should need to be done before a public release.
|
And yeah, docs + a test-case or two needed. Once that is there we can do a fine-comb review, but generally this is so remarkably tiny amount of code that I wouldn't expect any major issues there. |
|
Wrt implicit/explicitness of it all, it occurred to me that there are two quite distinct cases:
For 1. you want it to "just work" without further ado (it's already explicit in the spec afterall), but for 2. that needs to be a packager decision. One crude but relatively simple way would be using different directories for these, with the directory for 1. being outside the package builddir so upstream make system has "no access" (because it doesn't know where it is), and 2. in a well-known location in the build directory, that would only be used if explicitly enabled in the spec somehow. That conceptual split seems resolve the implicit/explicit problem for me (actual implementation is another question). You'd still want to programmatically tell whether a spec relies on this for sub-package generation though. One could easily flag it in the SRPM one way or the other, but not the spec alone for 1. One possibility could be (going back to) a new spec section that simply executes after %install, but without any special parsing. Then you can easily tell at spec parse whether this is used or not, like with |
|
When it comes to the distribution automagically generating sub packages I agree that the package need to be able to over write the behaviour. But I wonder if this really should be done on the "automatic sub packages off/on" level. Similar to what we have with debuginfo sub packages I'd rather expect each such feature providing some switch on its own. So packages would not switch off automatic sub package creation as a whole but the particular type that breaks. E.g. not splitting out precompliled (Python) files. |
|
I'd think there needs to be a global switch for at least case 2, but in addition individual scripts could/should be possible to disable case-by-case. Which might get tricky. Of course, just |
|
I guess I really need to write a proper design document... |
Please do. I'm clearly thinking on somewhat different lines from you. Which is not to say either one is wrong, just that there are so many possible scenarios that they're easy to miss entirely, and/or don't easily fit inside ones head simultaneously 😅 |
|
thanks @ffesti this groups |
|
Update? |
|
Added #2032 as the promised design document that lists a bit of the back ground. |
There was a problem hiding this comment.
Okay so to recoup the on-off discussions over many moons: there are further details to sort out. While we can work on the details later, the bare minimum requirements for merging are:
- documentation
- a test-case or two
For clarity, flagging as changes-needed for these two.
|
OK, added test case and some docs. |
Yep, there's no point duplicating the user docs in the commit message. What belongs in the commit message is a condensed version of the design spec: #2032 Because, commit messages are forever, a discussion on a proprietary platform is less so. |
|
...or maybe we should get into the habit of including these design docs in a directory someplace in the source tree. |
|
OK, fixed the error handling and added a failing test case. Turns out there is a reason that |
docs/manual/dynamic_specs.md
Outdated
| The files need to be placed in the **%{specpartsdir}** (also available | ||
| as **$RPM_SPECPARTS_DIR** in the build scripts) and have a | ||
| **.specpart** postfix. The directory is created by **%setup** in the | ||
| **buildsubdir**. Scripts must fail if it is not present. They should |
There was a problem hiding this comment.
This sentence sounds contradictory to what follows it, i.e. that they in fact don't have to fail if they do some kind of fallback.
There was a problem hiding this comment.
Reworded. There are just a little to many ifs and whens to make this nice to read literature. Or my poetic qualities are just lacking...
Do stuff for others to clean up is not an attitude us maintainers can afford you know. |
|
OK, addressed all the comments above except the issue with the signature of
|
|
|
Added alternative error handling. Turns out to be less of a change than I expected. So it's probably just the way to go. |
| rpmRC rc = RPMRC_OK; | ||
|
|
||
| char * specPattern = rpmGenPath("%{specpartsdir}", NULL, "*.specpart"); | ||
| if (rpmGlob(specPattern, &argc, &argv) == 0) { |
There was a problem hiding this comment.
One more thing I realized earlier today but ended up on a long detour... there's a subtle but important thing silently done by rpmGlob() here: it sorts the returned files. Without which, users of this feature may get a pretty weird spec salad of the day. I'd add a comment here because that's a not-so-known feature of rpmGlob(), and also document the order (sorted according to C locale) in the user documentation because it's something users will need to care about.
Yup, looks good. |
|
OK, seems like all the comments are now addressed. |
This allows parsing more spec pieces later in the build process
Don't require %end as first line when PART_NONE is "preceeding"
Read in *.specpart files from %{specpartsdir} aka $RPM_SPECPARTS_DIR
after the %install script. This allows the build process to add sub
packages to the build. In the future more changes to the packages may be
possible - like amending package declarations.
The %{specpartsdir} is created by the %setup pseudo macro inside of the
buildsubdir. It's presence allows build scripts to check if this feature
is supported in the running rpmbuild instance.
|
Rebased on top of #2270 and squashed latest fixes. |
This is an alternative to #1239
Instead of extending the spec syntax with dynamic elements allow the build process to generate their own spec file snippets that get read in after executing
%build(technically after%check) from$BUILDDIR/__rpm/*.spec.This allows tricks like generating sub packages from brp scripts - although I might need to think this through a bit more. While this could be used to implement debuginfo packages as brp script that might not be a great idea as they don't have easy access to the rpmbuiild internals. But for simpler use cases (e.g. language sub packages) this should be perfectly fine.
Another use case would be executing some X2rpm script that extracts the package structure from the manifest file of some scripting language module/project.