Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for declarative buildsystem #2774

Merged
merged 5 commits into from
Jan 15, 2024

Conversation

pmatilai
Copy link
Member

Building on the feedback from the #2620 draft, this seems more like it. Now even with documentation 😄

macros.in Outdated Show resolved Hide resolved
macros.in Outdated Show resolved Hide resolved
@voxik
Copy link
Contributor

voxik commented Nov 21, 2023

And in relation to #782, is it possible to define the 'buildsystem' inside of the .spec file?

@voxik
Copy link
Contributor

voxik commented Nov 21, 2023

And in relation to #782, is it possible to define the 'buildsystem' inside of the .spec file?

Although admittedly, not sure what would be the utility 🤔

@pmatilai
Copy link
Member Author

At least for now, these are just macros that can be defined anywhere. But I do fail to see the point of a spec defined buildsystem.

docs/manual/buildsystem.md Outdated Show resolved Hide resolved
@pmatilai pmatilai added RFE packaging Package building, SPEC files, etc. labels Dec 11, 2023
@pmatilai pmatilai marked this pull request as ready for review December 11, 2023 09:14
@pmatilai
Copy link
Member Author

Added docs for the global defaults + some other tweaks in that department, and added a cmake buildsystem example.

I think this is ready to go now. As with any new feature there's likely to be some rough edges, but those are best found in hands of the users rather than sitting in a draft somewhere on the edge of GH.

@pmatilai
Copy link
Member Author

pmatilai commented Jan 4, 2024

Oops, there was an unrelated extra commit from work related to #2803, must've gotten my local branches mixed up...

@dmnks
Copy link
Contributor

dmnks commented Jan 9, 2024

I'm a bit late to the 🎉 but as far as the overall solution, I'm giving the tag-based variant (BuildSystem:) implemented here a 👍 . I'm also giving a 👍 to the shorter Build* namespace (the Auto prefix really had to go 😄).

It looks to me like the cleanest and most flexible variant of all those mentioned previously, too. In particular, the possibility to interleave these with the other tags (such as BuildRequires:) makes it pretty neat as you can cluster those together if they're part of a conditional (as mentioned in the linked comment).

I personally hate whenever I need to look up the correct syntax for line breaks in a config format, this alleviates that problem since you just declare multiple of these tags. Yes, it's a bit noisy but somehow reminds me of CMake (as you also mentioned previously) and I guess I've developed the Stockholm syndrome already because I kinda like (or rather, don't mind) it too much 😆

As for the technical side of things, no opinion yet, I'm going to look at that next.

@dmnks
Copy link
Contributor

dmnks commented Jan 9, 2024

I personally hate whenever I need to look up the correct syntax for line breaks in a config format

OK, maybe this point is moot since I guess the line break syntax really is actually the SPEC's native one... but you get the point 😄

which case you might want to invoke the macros manually, eg.

```
%buildsystem_autotools_build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the least favorite part of mine, in terms of verbosity, but I suppose it can be tweaked later. Also, it's not the common happy path anyway so, as a packager needing an advanced/non-standard build procedure, once you open the lid and peek inside, you're already on your own anyway 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To elabore a bit (since I was quite unspecific above), what I mean by "verbose" is that this macro name consists of 3 separate parts, each of which I'm surely going to need to look up every time I use them (which, again, shouldn't be too often) 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... but ok, actually thinking about it (not just blindly looking), it's not rocket science - %buildsystem is the general namespace, _autotools is the build system name and _build is, well, the section name.

Copy link
Member Author

@pmatilai pmatilai Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. It's ugly, but we really need to namespace those macros so it can't be helped much.
For the case of multiple buildsystems in a single package that is.

For just manually invoking a random snippet from an active BuildSystem I do have a solution: macro aliases, where you only need to say eg %buildsystem_build or %buildsystem_install to manually invoke the right thing. That was part of an earlier version but left it out because the need for that case is much lower in this new, properly declarative system.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I like that. Good idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just spent a good part of the morning on that alias thing again, only to realize that it's largely useless because BuildOption's are not accessible from the spec, so manually invoking these macros in their long or the alleged shorter form is kinda pointless. Oh well.

@dmnks
Copy link
Contributor

dmnks commented Jan 9, 2024

Also, bonus points for being (perhaps accidentally) consistent with Flapak's naming (BuildSystem vs buildsystem): https://github.com/flathub/io.gitlab.osslugaru.Lugaru/blob/5580684b5524ddd63d289bbc06cc0305eae189b5/io.gitlab.osslugaru.Lugaru.json#L21

@pmatilai
Copy link
Member Author

pmatilai commented Jan 9, 2024

Oh. Any resemblance to Flatpak is purely coincidental, I swear 😄

An API that rpm itself doesn't use is likely to rot, as witnessed by
d376c6e08d7ccdc81e222d78d3eee097480af6e0. Also, this limits the number
of places directly accessing the spec, which is always a good thing.
Where there's more than one of a kind, sooner or later you'll want
to manipulate them programmatically. Use an array where array is
called for. No functional changes.

Note that the parsed spec contents is not a section, so we store it
separately.
Remove endless boilerplate to perform the same steps in each
package by just declaring which kind of buildsystem the package uses
using the new BuildSystem: spec tag.

Buildsystem macros are just regular parametric macros which can be invoked
manually from specs too, which should help with reuse and handling
complicated cases.

As such, this will only work for simple and/or "perfect" upstream projects
of course. In reality many/most packages need further tweaking, but this
can now be easily done with the new append/prepend modes of the build
scriptlets, and declaratively passed options to specific sections.

Add automake upstream amhello-1.0.tar.gz as an test-case of a rather
hands-free build with added distro specific doc.

Fixes: rpm-software-management#1087
We've had this special case for %clean for ages, but this actually
offers a nice opportunity to put it into a macro that allows customization
and is used for more than one purpose.

Drop the buildroot conditional - we always have one ever since rpm 4.6.
Split the examples into a separate macro file only used by the
test-suite.
@pmatilai
Copy link
Member Author

The last commit to not ship our example buildsystem macros could/should be merged into the "Implement..." commit instead, just made it separate to make it easy to back out if necessary.

@pmatilai
Copy link
Member Author

Okay I guess we've had all the feedback we're going to get on this.

@pmatilai pmatilai merged commit 0583808 into rpm-software-management:master Jan 15, 2024
1 check passed
@pmatilai pmatilai deleted the buildsystem-pr branch January 15, 2024 11:27
@Conan-Kudo
Copy link
Member

Thanks for finally landing this! 🚀

@pmatilai pmatilai added the highlight Release highlight label Mar 22, 2024
@dmnks dmnks changed the title Declarative buildsystem, take II Declarative buildsystem Mar 22, 2024
@dmnks dmnks changed the title Declarative buildsystem Add support for declarative buildsystem Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
highlight Release highlight packaging Package building, SPEC files, etc. RFE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants