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

Implement a declarative autobuild system (prototype) #2620

Closed
wants to merge 7 commits into from

Conversation

pmatilai
Copy link
Member

Remove endless boilerplate to perform the same steps in each package by just declaring which kind of buildsystem the package uses using the new Autobuild: spec tag.

Autobuild 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 in %install or otherwise. Augmenting those (one way or the other) needs to be implemented to make this real-world usable.

Add automake upstream amhello-1.0.tar.gz as an test-case of a rather hands-free autobuild.

Fixes: #1087

@pmatilai
Copy link
Member Author

Docs of course entirely missing as well, this is just an early rough-cut for feedback. The amhello.spec example + macros.in additions should make it fairly obvious though, it's not exactly rocket science.

@pmatilai pmatilai changed the title Implement autobuild "template" system (prototype) Implement a declarative autobuild system (prototype) Aug 22, 2023
@ffesti
Copy link
Contributor

ffesti commented Aug 22, 2023

We should probably put those build macros into separate macro files - one per build type. That makes it easier for people to add their own and to hand them over to other communities, if we want to get rid of them upstream.

Also the macros file is too big already.

@pmatilai
Copy link
Member Author

Sure, I just stuffed them there because... we don't have anything else at the moment.

@Conan-Kudo
Copy link
Member

I think this is going to need a versioned rpmlib capability, at least for SRPMs.

@pmatilai
Copy link
Member Author

pmatilai commented Aug 24, 2023

It fails cleanly on the unrecognized Autobuild: tag on versions where not supported. If we added rpmlib() deps for all of those, we'd have hundreds of them by now. rpmlib() deps are primarily about ability to install an rpm (whether source or not), and that's not a problem here.
That DynamicBuildRequires rpmlib() dep is kind of a hack (we needed something to mark the use of that feature), see eg 819c6c8.

@ffesti
Copy link
Contributor

ffesti commented Aug 24, 2023

This needs some design document or at least section in the commit message. This looks awfully like it could also be done with a simple macro or lua script. So we need some explanation why this isn't done that way.

@pmatilai
Copy link
Member Author

Which is what I said in the first comment:

Docs of course entirely missing as well, this is just an early rough-cut for feedback.

Using plain macros instead of writing them into files and then parsing is required to handle %prep which is special and using a simple macro just doesn't work, and also to supposedly allow full spec syntax with %if's and the like to be used in the autobuild macros. That doesn't work if we just drop in some macros at the end of the parse, which is the simplest possible approach of course.

At this point I'm more interested in the mechanism and what might be missing. Such as the excellent point about BuildRequires from @voxik (in the ticket).

@ffesti
Copy link
Contributor

ffesti commented Aug 24, 2023

The BuildRequires case is just one example of Preamble content. The dynamic spec feature will soon drop you into the preamble of the main package for each parsed file (PR coming soon). The other option (which is possible now) is to expand a macro directly where the Autobuild directive is encountered - which is also in the main preamble.

@pmatilai
Copy link
Member Author

pmatilai commented Aug 24, 2023

Yes, implementing the BuildRequires part isn't hard. For autobuild I do not want "insert arbitrary preamble section here" feature, that's not it's goal at all. Automating package generation is a separate topic.

@pmatilai
Copy link
Member Author

pmatilai commented Aug 24, 2023

Oh and FWIW, I initially thought of just dropping these pieces into the specpartdir and let the dynamic spec parsing handle it, but that creates a chicken-egg problem: those generated spec parts require a build to happen first, and that build doesn't happen unless the autobuild stuff is injected into the spec at parse time already.

@ffesti
Copy link
Contributor

ffesti commented Aug 24, 2023

Yeah, this needs to be non dynamic. But it can use dynamic spec in the implementation of the different build types. So this gets confusing quickly.

@pmatilai
Copy link
Member Author

pmatilai commented Aug 24, 2023

Not just can, but it relies on the dynamic spec parsing machinery to handle the out-of-band content at the moment (that bit of refactoring to split out some logic out of parseGeneratedSpecs(), which needs to be split to a separate commit). That's an implementation detail users dont need to care about though.

Oh and okay, you mean that it can use dynamic generation in addition to that. Yep.

@pmatilai pmatilai force-pushed the templ-pr branch 2 times, most recently from 5611b97 to 4a879f0 Compare August 25, 2023 11:51
@pmatilai
Copy link
Member Author

pmatilai commented Aug 25, 2023

Some git hygiene applied to the previous work + rationale added to the commit message, BuildRequires support added. It probably should be squashed before eventual merging but thought the changes are easier to see this way. Doing the buildrequires like this does feel somehow clunky and ugly but we can't generate preamble content from here AFAICS.

@pmatilai
Copy link
Member Author

One thing I just realized is that there's no reason for each section to be in a different file. We'll still need a randomized tmp filename for it but it could at least hint at its purpose by autobuild prefix or such.

We'll need this on other paths than specpartsdir in the next commits.
No functional changes.
@pmatilai
Copy link
Member Author

pmatilai commented Oct 27, 2023

Rebased on current master and updated the test-case to showcase a more real-world use scenario with %build -a. Docs still missing of course, I'll do that next.

This is basically fully functional as-in though, the macro alias stuff is more of an added bonus than a strict requirement.

Remove endless boilerplate to perform the same steps in each
package by just declaring which kind of buildsystem the package uses
using the new Autobuild: spec tag.

Autobuild 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.

The implementation looks more complicated than one might expect because
we want the full spec processing to take place on these parts, rather
than just macro expansion. That covers things like the special facilities
of %prep and their side-effects, %if-conditionals, possible error
handling and so on.

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

Fixes: rpm-software-management#1087
@pmatilai
Copy link
Member Author

Also, if we ship any default autobuild macros at all, we'll need to include something for cmake too. Shipping autotools macros but no cmake macros, in a cmake project, would look a bit odd 😅

@Conan-Kudo
Copy link
Member

It does probably make sense to pull the CMake macros into rpm so we can build ourselves from what is provided by rpm itself... 😅

@gotmax23
Copy link
Contributor

gotmax23 commented Oct 27, 2023

It would be nice if we could also use this system to generate a %generate_buildrequires section in addition to generating static BuildRequires.

EDIT: It is not lost on me how many times I said generate in that last sentence...

macros.in Outdated
@@ -1349,5 +1349,14 @@ end
end
}

# example autobuild macros for autotools
%autobuild_autotools_prep() %autosetup -p1
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand %prep being part of this. It's not going to differ from one build system to another, and including it in the %build -a steps increases the number of spec files that won't be able to use autobuild because they need to perform some sort of pre-build shenanigans. (Running sed on some of the extracted files, extracting additional %SOURCEN tarballs, etc...)

Copy link
Member Author

@pmatilai pmatilai Nov 1, 2023

Choose a reason for hiding this comment

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

Sure, the %prep part is here more for demo purposes than something we'd want to ship for real. Instead we should probably just automatically add "%autosetup -p1" %prep section to everything that has sources and does not have a prep. I don't understand the remark about %build -a though. If you need to do pre-build shenanigans, you do it in one %prep -a for the earliest opportunity (assuming automatically added %prep %autosetup) , or in %conf -p/-a or %build -p, as appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, there was a reason %prep is a part of this: language ecosystems may have source formats outside the tarball realm, and in those cases we'd rather just have the ecosystem define %prep as they see fit, using their native tools. Ruby is an example here - rpm shouldn't have to know about .gem.

As for the rest, we could indeed just autopopulate %prep with a generic "%autosetup -p1" when Autobuild is set, but I'm not sure it's somehow better than doing it explicitly in the autobuild definition. It's just one line in the definition, and there could be other ecosystem preferences I guess. Thoughts welcome certainly.

Copy link
Member Author

Choose a reason for hiding this comment

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

...and that's what the last fixup does: add autobuild defaults that will be applied if there's no build-system specific section. Only probably makes sense for %prep and %clean but the mechanism is generic so supporting the all makes no difference in code.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 1, 2023

Another note here is that in this draft, the autobuild_* macros are parametric, but that's probably a bad idea - if %configure was parametric, it'd be useless because you'll want to pass arbitrary --enable-this/--disable-that switches...

@Conan-Kudo
Copy link
Member

Would it make sense to be able to expose some kind of simple way to pass options to the configure stanza? Autotools, CMake, and Meson at least all accept configure options, and it'd make sense to be able to do this without having to override the whole stage.

A similar example of something like this is in Flatpak, where you declare the buildsystem type and then declare the options to be passed in. For example Lugaru's manifest sets the buildsystem to "cmake-ninja" and then has a field for setting extra options.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 1, 2023

It would be nice if we could also use this system to generate a %generate_buildrequires section

You can. All build scriptlets are supported.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 1, 2023

Some early docs added, fixed typo/thinko wrt %generate_buildrequires name, and dropped the parametric requirement.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 2, 2023

Would it make sense to be able to expose some kind of simple way to pass options to the configure stanza? Autotools, CMake, and Meson at least all accept configure options, and it'd make sense to be able to do this without having to override the whole stage.

The thought crossed my mind too, we could of course add an optional %{autobuild_mumble_opts} macro that can be used for passing extra options. I'm not sure how much more readable it is than overriding the section though (which is why it's not in the draft):

%conf
%autobuild_conf --add-some-option 2

vs

%define autobuild_conf_opts --add-some-option 2

The latter is more in line with the notion of declarative, so perhaps that's the way to go. Without taking away the ability to override sections.

@Conan-Kudo
Copy link
Member

Why not define it as a field instead of a macro?

@pmatilai
Copy link
Member Author

pmatilai commented Nov 2, 2023

You mean a spec tag, like AutobuildConfOpts: --some-opts here?
I dunno. The tags look even uglier to me than the macros, and macros is what the implementations need to deal with anyhow.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 2, 2023

There would need to be a way to pass options to all the autobuild macros, not just conf. Because sometimes you need that extra FU=1 SOMEDIR=/there to a make invocation etc, and ... of course the configure / make style line might not be the last thing in the autobuild macro. And so .. maybe these do need to be parametric afterall, to enforce a common convention for passing stuff, so we don't end up with one macro accepting anything passed after it and another one throwing an error.

And thanks for the feedback so far everybody!

@Conan-Kudo
Copy link
Member

You mean a spec tag, like AutobuildConfOpts: --some-opts here? I dunno. The tags look even uglier to me than the macros, and macros is what the implementations need to deal with anyhow.

I guess, it's just that a spec tag field is standardized, whereas a macro is not. Macro names can be anything at all and there are no real rules or discovery mechanisms for them. And if we're talking about handling the basic case in a reasonably sane way, then being able to pass autobuild configuration options in a uniform fashion would be nice.

I'm not totally married to the idea of it being a spec tag though. It just feels like doing so would enforce more consistency.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 2, 2023

The macro names here would be simply "%autobuild_build_opts", "%autobuild_conf_opts" and so on, entirely predictable, and enforced by the implementation. But I agree tags are more, hmm, concrete somehow. I'll think about it 👍

@pmatilai
Copy link
Member Author

pmatilai commented Nov 3, 2023

Another remark on the per-section options (configuration being the most important of course) is that whatever the method/syntax, it needs to handle multiple long arguments gracefully. Because those config parameters can get long. Just on a random sampling, xterm in fedora has the following:

%configure \
        --enable-meta-sends-esc \
        --disable-backarrow-key \
        --enable-exec-xterm \
%{?with_trace: --enable-trace} \
        --enable-warnings \
        --with-app-defaults=%{x11_app_defaults_dir} \
        --with-icon-theme=hicolor \
        --with-icondir=%{_datadir}/icons \
        --with-utempter \
        --with-tty-group=tty \
        --disable-full-tgetent \
        --with-pcre2 \ 
        --enable-readline-mouse \
        --enable-logging

...and %configure entries of this length are in no way rare. And what's acceptable as a long formatted line-continuation when passed to %configure like this is not acceptable as a tag. As a macro body its more doable but still ugly - for declarative style they really should be separable entities and not just a heap of text. So, perhaps there should instead be macro(s) that allow you to add options, in a controlled fashion. Something along the lines of:

%autobuild_option conf --enable-meta-sends-esc
%autobuild_option conf --with-icondir=%{_datadir}/icons

@Conan-Kudo
Copy link
Member

AutobuildOption(conf): could just be a repeatable tag, no? That's how the ExclusiveArch, ExcludeArch , and the dependency tags work.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 3, 2023

Indeed - I was just coming back here to add that alternative 😅

@pmatilai
Copy link
Member Author

pmatilai commented Nov 3, 2023

The downside of tags is that they force the implementation to the C-side of things, whereas I'd rather have this in the (Lua) macro space. OTOH, the outcome has to be the more important factor than implementation preferences.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 3, 2023

Yet another random remark is that for this declarative style, "autobuild"-prefix creates annoyingly long names without being particularly descriptive. "Buildsys" comes to mind, but it only saves a couple of letters and I'm not sure it's significantly more descriptive anyway. Dunno.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 6, 2023

For a comparison of various alternatives, nothing works better than a real-world test (using an excerpt from Fedora xterm.spec):

  1. Just override the section
%conf
%autobuild_conf \
        --enable-meta-sends-esc \
        --disable-backarrow-key \
        --enable-exec-xterm \
%{?with_trace: --enable-trace} \
        --enable-warnings \
  1. Spec tags
AutobuildOption(conf): --enable-meta-sends-esc
AutobuildOption(conf): --disable-backarrow-key
AutobuildOption(conf): --enable-exec-xterm
%if %{with trace}
AutobuildOption(conf): --enable-trace
%endif
AutobuildOption(conf): --enable-warnings
  1. "Add option" style macros (kinda like cmake actually)
%autobuild_option conf --enable-meta-sends-esc
%autobuild_option conf --disable-backarrow-key
%autobuild_option conf --enable-exec-xterm
%if %{with trace}
%autobuild_option conf --enable-trace
%endif
%autobuild_option conf --enable-warnings
  1. One simple _opts macro (with various caveats wrt embedded newlines)
%define autobuild_conf_opts \
        --enable-meta-sends-esc \
        --disable-backarrow-key \
        --enable-exec-xterm \
%{?with_trace: --enable-trace} \
        --enable-warnings
  1. A section for declaring options, line by line (similar to eg %patchlist)
%autobuild_conf_options
--enable-meta-sends-esc
--disable-backarrow-key
--enable-exec-xterm
%if %{with trace}
--enable-trace
%endif
--enable-warnings

2-3, ie the ones most discussed above, are the ones that actually add a lot of technically redundant clutter to the spec. They're are quite readable though, and although it's not evident here, they have the (non-trivial) benefit of being relatively freely placeable within the spec: in many/most cases you'd be able to place the option right alongside eg extra buildrequires for that option. 3) is even more freely placeable than 2.

5 is the only one that really reduces boilerplate and ugly + brittle line-continuations, at the cost of forcing the declarations to one spot again, which is a considerable loss compared to 2-3. It's also way less obviously independent entries and looks more like a continuous text, which makes you think there are line continuations missing or something.

4 is only a win over 1 if one assumes complicated autobuild macros that expand into more than one command so you can't just continue the line as you'd do with eg %configure.

So it's a curiously contradicting set of possibilities, and despite being more "wordy", 2-3 have significant advantages over the seemingly more simple versions.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 6, 2023

For a real-world examle of the 2-3 benefit, a typical bcond case from Fedora rpm.spec:

Existing spec:

%bcond_without libarchive
[...]
%if %{with libarchive}
BuildRequires: libarchive-devel
%endif
[...]

%prep
cmake \
   [...]
   %{!?with_libarchive:-DWITH_ARCHIVE=OFF}\

Using a section override for %conf would be effectively unchanged from existing.

Spec tags:

%bcond_without libarchive
[...]
%if %{with libarchive}
BuildRequires: libarchive-devel
AutobuildOption(conf): -DWITH_ARCHIVE=OFF
%endif

3 would work equally well, just look a little out of place in middle of those spec tags. In some other situation it may be surrounded by other macros and a tag would look out of place, but that's probably an uncommon situation, actually. The above also screams to beBuildOption(conf) instead of this auto-silliness.

Yet another observation is that configuration is the only place where one typically expects "dozens" of options. For actual build/install stages, it's usually more a matter of adding a flag / argument or two to override some specific thing. Or, that's what I thought. Looking at the Fedora spec collection, there's all manner of stuff being passed to both %make_build and %make_install (and never mind everything that falls outside these). So while I initially thought perhaps only configuration deserves the 2-3 style arrangement and others could do with just a simple _opts macro (ie 4), perhaps not so.

@pmatilai
Copy link
Member Author

pmatilai commented Nov 6, 2023

So... the conclusion from all the above rant is that there seems to be an actual design wanting to come out:

BuildOption(section): to add options to the sections in a truly declarative fashion, and as a shortcut for the most common section, BuildOption: without the parentheses equals BuildOption(conf):. And call the whole thing by some different name. BuildType: perhaps, but that could have other uses (eg debug/production or such). BuildSys:, BuildSystem: or just even just Build: maybe.
Thoughts welcome.

Too bad there is no option to convert a PR into a discussion because that's what this resembles more now 😄

@pmatilai
Copy link
Member Author

pmatilai commented Nov 6, 2023

Ok, back to drawing board, now with a much nicer plan.

Thanks for the very valuable feedback + ideas!

@pmatilai pmatilai closed this Nov 6, 2023
@Conan-Kudo
Copy link
Member

BuildSystem, BuildType, and BuildOption(stage) makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFE: Declarative build system support
5 participants