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

Dynamic Build Dependencies #593

Merged
merged 6 commits into from May 28, 2019

Conversation

Projects
None yet
8 participants
@ffesti
Copy link
Contributor

commented Nov 6, 2018

As discussed in #104 this allows adding a %generate_buildrequires section to spec files. The sections stdout is read in as BuildRequires and are checked. If some are missing the build is aborted.

This is still in a prove of concept state. The dependencies added to the SRPM for this features need to be thought through more thoroughly. Stopping the build for missing dependencies also deserves its own return code of rpmbuild.


Expected behavior:

  • rpmbuild -bs
    • do not check any dependencies
    • generate standard src.rpm without dynamic BuildRequires (but with "Requires: rpmlib(DynamicBuildRequires)")
  • rpmbuild -br
    • no %generate_buildrequires supplied → write src.rpm ($status = 0)
    • check standard (manually-specified) BuildRequires
    • check generated BuildRequires
      • some are missing → write buildreqs.nosrc.rpm ($status = 11)
      • none are missing → write src.rpm (with "Provides: rpmlib(DynamicBuildRequires)", $status = 0)
  • rpmbuild -br --nodeps
    • no %generate_buildrequires supplied → do nothing ($status = 0)
    • run %generate_buildrequires script and write buildreqs.nosrc.rpm ($status = 11)
  • rpmbuild -bb
    • run %generate_buildrequires script if present and check dependencies
      • some are missing → write buildreqs.nosrc.rpm ($status = 11)
  • rpmbuild -bb --nodeps
    • skip %generate_buildrequires script if present
  • rpmbuild -ba
    • run %generate_buildrequires script if present and check dependencies
      • some are missing → write buildreqs.nosrc.rpm ($status = 11)
      • none are missing → write src.rpm and continue build
  • rpmbuild -ba --nodeps
    • run %generate_buildrequires script if present and just use that information in final src.rpm
    • build as usual

Known issues:

  • Invalid dependency errors are not giving enough information (

    rpm/build/parseReqs.c

    Lines 335 to 341 in e8fce62

    /* Automatic dependencies don't relate to spec lines */
    if (tagflags & (RPMSENSE_FIND_REQUIRES|RPMSENSE_FIND_PROVIDES)) {
    rpmlog(lvl, "%s: %s\n", emsg, r);
    } else {
    rpmlog(lvl, _("line %d: %s: %s\n"),
    spec->lineNum, emsg, spec->line);
    }
    )
error: line 97: invalid dependency: - Initial package
  • Providing rpmlib(DynamicBuildReqiures) is broken (

    rpm/build/reqprov.c

    Lines 22 to 26 in 8f509d6

    /* rpmlib() dependency sanity: only requires permitted, ensure sense bit */
    if (rstreqn(N, "rpmlib(", sizeof("rpmlib(")-1)) {
    if (tagN != RPMTAG_REQUIRENAME) return 1;
    Flags |= RPMSENSE_RPMLIB;
    }
    )
@ffesti

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Oh, rpmbuild also needs to grow a cli param to control the execution of the %generate_buildrequires section.

Show resolved Hide resolved build/parseSpec.c Outdated
@ffesti

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Thinking a bit more rpmbuild should probably continue building the srpm with the generated BuildRequires included (probably depending on command line options) even when failing the build of the binary packages due to missing BuildRequires. So build systems could setup the second attempt using the newly build srpm.

@ffesti

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Fixed. Nice catch!

@pmatilai

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

Still haven't gotten around to look this in any real detail, but here's another thought: why limit this to only buildrequires? One of the major shortcomings of the "new" dependency generator is the inability to generate package specific dependencies easily, and this looks like a nice general approach to that. So perhaps this should be %generate with an argument to specify which kind of dependencies are being generated (buildrequires, requires, provides...).

@Conan-Kudo

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

Still haven't gotten around to look this in any real detail, but here's another thought: why limit this to only buildrequires? One of the major shortcomings of the "new" dependency generator is the inability to generate package specific dependencies easily, and this looks like a nice general approach to that. So perhaps this should be %generate with an argument to specify which kind of dependencies are being generated (buildrequires, requires, provides...).

That would be tremendously useful for cases where runtime deps might need to be generated in a package specific way. For example, a Ruby application may have a Gemfile to indicate its dependencies, but it isn't built as a Ruby Gem. As a consequence, there's no way to process those dependencies through the current dependency generator system. A lot of Python applications also do this by shipping Pipfile, pyproject.toml, or even just a requirements.txt file for this. These files don't get installed on the system, and they generally don't produce metadata at install time, so the only way to generate deps from them is by reading the file and processing it at build time.

It'd be great if we could have support for that too.

@ffesti

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

I am not at all against having scripts for normal dependency generation. But their are a few important differences to the BuildRequires:

  • They have to be attached to a sub package and cannot be global
  • We may want to (be able to) associate the script to files - so we need a way of passing the fileattr macros (path, flags, magic).
  • They don't need the funky stop build, install new deps and restart build cycle
    So they need to be done a different way as the %generate_buildrequires script anyway. So while I agree we should add those it is probably best to concentrate on the topic at hand first.

Also not sure if %generate_buildrequires vs %generate buildrequires really makes a lot of difference.

@pmatilai

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

I'd simply leave the file associations out of this style of dep generation entirely, it'd be for the cases where the file associations don't make sense which is quite a common case actually. The %generate-script could take the subpackage as an optional argument, similarly to other spec sections.

The script execution should be refactored to use common code with doScript(), this is duplicating a large part of it (which is ok for PoC of course). Not sure if templates make sense here, but then again why not: the information provided there (name, version etc) are commonly needed in generators too.

As for the src.rpm generation: I guess it should only fail to create src.rpm in case %generate_buildrequires script itself fails, due to eg missing external tools needed to extract the data, (such tools should be static buildrequires). I guess it should also generate those dynamic buildrequires even in the case where just src.rpm is being generated. Would be a fairly big change of course since currently src.rpm creation runs no scripts, but if it worked that way build systems would require less changes. Hmm, but then src.rpm creation might need to honor static buildrequires again...

@nim-nim

This comment has been minimized.

Copy link

commented Nov 30, 2018

I am not at all against having scripts for normal dependency generation. But their are a few important differences to the BuildRequires:

* They have to be attached to a sub package and cannot be global

* We may want to (be able to) associate the script to files - so we need a way of passing the fileattr macros (path, flags, magic).

* They don't need the funky stop build, install new deps and restart build cycle
  So they need to be done a different way as the %generate_buildrequires script anyway. So while I agree we should add those it is probably best to concentrate on the topic at hand first.

Also not sure if %generate_buildrequires vs %generate buildrequires really makes a lot of difference.

From a human interface POW both are awful

I can name the macro designed to be executed in prep %goprep, the one designed to be executed in build %gobuild and so on, but
%go_generate_buildrequires ? A third of the line is gone without even injecting a single argument

And go is lucky, it only needs two-letters for prefixing, other languages/ecosystems eat more letters.

@nim-nim

This comment has been minimized.

Copy link

commented Nov 30, 2018

That being said, the langage br computation is done in
nim-nim/go-macros@52c4bcc

it can be plugged in a mock/rpm stack as soon as this part is ready

nim-nim pushed a commit to nim-nim/go-macros that referenced this pull request Nov 30, 2018

nim-nim pushed a commit to nim-nim/go-macros that referenced this pull request Nov 30, 2018

@Conan-Kudo

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

@nim-nim you could just do %go_genbuildreqs

@nim-nim

This comment has been minimized.

Copy link

commented Nov 30, 2018

Yes sure I can shorten it my side, and then others will shorten it with a slightly other convention their side, and it will all end up a huge mess.

It would be much better if the naming root was shorter in the first place so others do not have to shop it. That's why prep is much nicer than preparation.

@xsuchy

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

What exit code rpmbuild returns when a build fails because of %generate_buildrequires? I cannot find it in a code.

@xsuchy

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

Mock will have to parse output of rpmbuild - in case the rpmbuild fails due %generate_buildrequires. Parsing the output for _("Failed build dependencies:\n") is suboptimal. Especially because of _() and because rpmpsPrint() use fprintf(f, "\t%s\n", msg);. Can I have something machine friendly? Either:

  • --parse - Optimize the command output for easy parsing. Where the packages are on one line, comma separated. And the line is not localized.
  • --generate_buildrequires-to-file FILE which will put the needed build requires to separated file. (This needs to be documented how to interpret --root option)
  • or something else
@nim-nim

This comment has been minimized.

Copy link

commented Dec 3, 2018

Mock will have to parse output of rpmbuild - in case the rpmbuild fails due %generate_buildrequires. Parsing the output for _("Failed build dependencies:\n") is suboptimal. Especially because of _() and because rpmpsPrint() use fprintf(f, "\t%s\n", msg);. Can I have something machine friendly? Either:

* `--parse` - Optimize the command output for easy parsing. Where the packages are on one line, comma separated. And the line is not localized.

* `--generate_buildrequires-to-file` `FILE` which will put the needed build requires to separated file. (This needs to be documented how to interpret `--root` option)

* or something else

I’m not sure the comma separated line is a good idea, sooner of later someone will need to generate build dependencies with complex constrains (at least version constrains, maybe even rpm rich deps). One failing construct per line is always going to be simpler to understand.

And the file option is probably going to be a bit too restrictive for some use cases.

How about just prefixing each missing build dependency line with a prefix you can lock on? Just

Failed build dependencies:(localised)
BuildDep: <some rpm dep construct>
BuildDep: <some rpm dep construct>

You only need to make sure rpmbuild never outputs a line starting with BuildDep: for any other reason when failing with the %generate_buildrequires failure error code.

@ignatenkobrain

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

So in order to make some move here, I've submitted Fedora Change here: https://fedoraproject.org/wiki/Changes/BuildRequires_Generators

@nim-nim

This comment has been minimized.

Copy link

commented Feb 24, 2019

Yes it would be really nice to get to the point where we can test generator integration within rpm !

Show resolved Hide resolved build/build.c Outdated
Show resolved Hide resolved build/build.c Outdated
@praiskup

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@xsuchy wrote:

What exit code rpmbuild returns when a build fails because of %generate_buildrequires?

I'd say it can be non-zero, as mock doesn't necessarily have to expect a "specific" exit status in this case. Yes, specific exit status would make it easier for mock, but I fail to see what are the expected rpmbuild exit statuses from it's manual page, unfortunately and it seems to be so far undefined.

--generate_buildrequires-to-file

The detection whether RPM supports this option would be ugly hack in mock. What about to have %dynamic_buildrequires_file macro defined by rpm itself? rpmbuild should only make sure the file is removed before its each run. Mock then can do - upon any failure:

dbf=$(rpm --eval "%dynamic_buildrequires_file")
test -n "$dbf" && test -f "$dbf" && do_the_install_logic_and_try_rpmbuild_again

@nim-nim

I’m not sure the comma separated line is a good idea,

+1.

How about just prefixing each missing build dependency line with a prefix you can lock on?

-1. I think that rpmbuild should assure that the shape of the output file is as deterministic as possible, no matter whether the packages are or are not already installed. I think it should be just a list of build requires without any prefix or so, without any header message, no warnings, etc. As easy as possible.

@xsuchy

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Yes, specific exit status would make it easier for mock, but I fail to see what are the expected rpmbuild exit statuses from it's manual page, unfortunately and it seems to be so far undefined.

Yes. I checked the code and rpmbuild always fail with
_exit(EXIT_FAILURE)
which has value 1.

I already started working on this feature in Mock. I will have this exit code there as constant, so it will be easy to change it later.

@nim-nim

This comment has been minimized.

Copy link

commented Mar 12, 2019

@praiskup

-1. I think that rpmbuild should assure that the shape of the output file is as deterministic as possible, no matter whether the packages are or are not already installed. I think it should be just a list of build requires without any prefix or so, without any header message, no warnings, etc. As easy as possible.

I think there are two different things here, how you format rpmbuild error output to stdout/stderr, and how you format the rpmbuild data output to other tools.

@praiskup

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@nim-nim

This comment has been minimized.

Copy link

commented Mar 12, 2019

On Tuesday, March 12, 2019 9:55:29 AM CET nim-nim wrote: I think there are two different things here, how you format rpmbuild error output to stdout/err
Why we should care about 'rpmbuild' stdout/stderr here?

Because people do write scripts that call rpmbuild. It's a lower level of coupling than the one used by mock & co, but it exists.

@eclipseo

This comment has been minimized.

Copy link

commented Mar 21, 2019

Great work Florian, I hope to see this merged soon!

(Not so keen on %generate_buildrequires, it's a bit long)

@ffesti ffesti force-pushed the ffesti:builddeps branch from a5707b3 to 30fed42 Apr 5, 2019

@ffesti

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Ok, here is the next iteration.
rpmbuild now exits with 11 in case of not met dynamic build dependencies that require re-running the build after installing them.
In this case rpmbuild creates a .buildreqs.rpm package which basically is a srpm which includes the dynamic build dependencies as Requires but does not include the sources (expect the spec file). I may remove more things from that package if it gives a significant performance improvement (%changelog being candidate No. One).
Users (including build tools like mock) are expected to install the dependencies of the .buildreqs.rpm and the restart the build. e.g. with rpmbuild --noprep -ba (or -bb). This can go over multiple iterations. Not being able to install a new dependency should be seen as an error, though, and lead for the building attempt to be stopped.

rpmbuild now supports -br -tr -rr for creating an srpm including the dynamic build dependencies. Note that this requires the static build dependencies and also the rebuild loop just as for building a binary package.

@ignatenkobrain

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I did give this quick try:

  1. Wrote: /home/brain/rpmbuild/SRPMS/rust-warp-0.1.15-1.fc31.buildreqs.rpm feels a bit strange to have in SRPMS directory with Architecture: x86_64. Probably we should not set Architecture for these RPMs?
  2. rpmbuild --rebuild /home/brain/rpmbuild/SRPMS/rust-warp-0.1.15-1.fc31.src.rpm doesn't have exit code 11 when dependencies are missing. It has return code 0.
  3. It seems that dnf builddep doesn't work with this buildreqs.rpm which will block mock work a bit. no package matched: /home/brain/rpmbuild/SRPMS/rust-warp-0.1.15-1.fc31.buildreqs.rpm
@pmatilai
Copy link
Contributor

left a comment

As per above, changes requested. Hopefully this resets the review state properly, things have gotten a bit confused here (my bad)

@ffesti ffesti force-pushed the ffesti:builddeps branch 3 times, most recently from 958a013 to e42c0a3 May 15, 2019

@ffesti

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

OK, that should be the requested changes. Still missing is squashing of the three patches from "Pass rpmts object to rpmSpecBuild" onward and a test case or two.
Patch set also get rebased to current head.

Show resolved Hide resolved build/build.c Outdated
Show resolved Hide resolved build/build.c
Show resolved Hide resolved build/rpmbuild.h
@pmatilai

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

As the GH interface doesn't allow commenting on individual commits, I'll just make a general comment... In the "Pass rpmts object to rpmSpecBuild()" commit, there are two minor issues:

  • a stray empty line added before buildSpec()
  • a change to rpmbuild.c that belongs to the next commit (move to library, return 11 on missing buildrequires)

Other than that, and the missing tests, it's looking quite okay now.

@ffesti

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Found one more issue: --quiet also makes the BuildRequires disappear as the output of all build scripts is piped to /dev/null. OK, when exactly are those macros expanded...?

Show resolved Hide resolved build/build.c Outdated

@ignatenkobrain ignatenkobrain force-pushed the ffesti:builddeps branch from cc8b986 to ec9391b May 24, 2019

ffesti added some commits Apr 10, 2019

Pass rpmts object to rpmSpecBuild()
Add rpmtsFromPyObject
No functional change. Needed to move BuildRequires check to librpmbuild
Move check for build dependencies out of rpmbuild.c and into build/bu…
…ild.c

Exit with return code 11 on missing build dependencies.

@ffesti ffesti force-pushed the ffesti:builddeps branch 2 times, most recently from 7bacaa2 to 8357199 May 27, 2019

@ffesti

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

Ok, --quiet issue is fixed. Thank to Igor for helping with this!
Test are also added - and should now also include the new spec and tar ball...

@pmatilai

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

Some minor tweaks to do still:

  • Squash the tests to the commit that adds the feature, that's where they belong
  • There's an way too long parseRCPOT() line in doBuildRequires() for loop still, not sure if it's been mentioned before, if not sorry for missing on previous rounds
  • Clean up the test spec a bit, please. We shouldn't really add any new references of these...
+Vendor: Red Hat Software
+Packager: Red Hat Software <bugs@redhat.com>
...
+%changelog
+* Tue Oct 20 1998 Jeff Johnson <jbj@redhat.com>
+- create.

ignatenkobrain and others added some commits May 24, 2019

Do not redirect command output to /dev/null in non-verbose mode
The output is going to be needed Dynamic BuildRequires.
Just do not duplicated to stdout if verbose mode is not enabled.

Signed-off-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
Add support for dynamic BuildRequires
Supports new %generate_buildrequires section in the spec file which is executed
after %prep. Stdout is captured and turned into BuildRequires. These are then
checked. If they cannot be fulfilled a source package is created with all
BuildRequires and the build is terminated after that.

rpmbuild has now the following new build modes -br, -tr, -rr and exits with 11
if build requirements are not met.

That means for users:
* No %generate_buildrequires
  * rpmbuild -br is equivalent to rpmbuild -bs
  * rpmbuild -br --nodeps is equivalent to rpmbuild -bs
* %generate_buildrequires
  * rpmbuild -br will check dynamic BuildRequires
    * Satisfied → src.rpm
    * Unsatisfied → buildreqs.nosrc.rpm
  * rpmbuild -br --nodeps will always generate buildreqs.nosrc.rpm

Source packages contain
Requires: rpmlib(DynamicBuildRequires) <= 4.15.0-1
if the spec contains a %generate_buildrequires section and
Provide: rpmlib(DynamicBuildRequires) = 4.15.0-1
if the results been added to the source package.

Co-authored-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>

@ffesti ffesti force-pushed the ffesti:builddeps branch from 8357199 to 6d140a5 May 27, 2019

@pmatilai

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Okay, I think that was all. BTW @ffesti, please add at least a brief comment when pushing updates, I only noticed this had been updated by accident this morning when it could've been merged yesterday already.

@pmatilai
Copy link
Contributor

left a comment

Looking fine now.

@pmatilai pmatilai merged commit 58dcfdd into rpm-software-management:master May 28, 2019

1 check passed

semaphoreci The build passed on Semaphore.
Details
@xsuchy

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Awesome. Thank you, guys!

@nim-nim

This comment has been minimized.

Copy link

commented May 28, 2019

Fantastic, thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.