Fix regression: -bp should check BuildRequires

[PaulSD]

Some source packages assume that BuildRequires are installed before %prep is executed. For example, openssh BuildRequires automake and runs autoreconf in %prep.

Prior to 11c56d5 rpmbuild -bp checked BuildRequires and printed a helpful error message if there were any missing dependencies. Since that commit, rpmbuild -bp skips the BuildRequires check, which can lead to misleading errors in %prep.

This commit restores the BuildRequires check in rpmbuild -bp.

[PaulSD]

... Looking at this again more closely ... It looks like a the BuildRequires checks have been moved after %prep in a lot of other places too, so this PR is insufficient...

However, it looks like discussion is warranted before going any further down this path.

For RHEL9 (rpmbuild 4.16.1.3), %conf doesn't exist yet, so I think it makes sense to do the BuildRequires checks before %prep to avoid issues like the ones I experienced with existing packages.

However, for rpmbuild >=4.18.0, we now have %conf. I think the current behavior of doing the BuildRequires checks after %prep and before %conf makes sense, and we should probably consider the openssh behavior to be a bug in the openssh SPEC file (it should run autoreconf in %conf instead of %prep).

So ... would it make sense to rework this PR to only apply to the 4.16 branch for RHEL9?

[pmatilai]

Hmm, %prep not checking buildrequires does sound like a bug, or rather, a regression. While %prep is mainly for unpacking sources and applying patches, that can require custom tools just like any other step. Such as that autoreconf. If that's affected I'm quite surprised this hasn't turned up before now... @ffesti you might want to take a look at this.

[ffesti]

There are two different things to consider: When are BuildRequires checks done during the build process and whether to do it depending on what stage/subcommand are executed. Looking at https://github.com/rpm-software-management/rpm/blob/master/build/build.c#L379 and https://github.com/rpm-software-management/rpm/blob/master/build/build.c#L388 the BuildRequiresCheck seems to be executed at the right time.

This case statement in rpmbuild.c with all its fallthroughs is a bit difficult to read, but the patch is IMHO correct in putting the BuildDependency check at the bottom of the fall through block - enabling it also for -bp.

There is an argument to be made that the BuildDependency check should also be made when --short-circuit is given (which it is currently not (except for prep after this patch or -bf before) but may be this is a separate issue.

So I don't see any other sub commands that should have trouble with the BuildDependency check not being executed properly (assuming the short-circuit issue is ignored). So the patch should actually be sufficient IMHO.

[ffesti]

OK; after looking at the failing test case it's a bit more obvious that we need to treat the dependency check and the %generatebuilddependency script differently. While the former needs to move to the case 'p'(rep) block the later needs to stay where it is at it is not needed for %prep at all. Let me just fix that.

[pmatilai]

The code looks simple enough, but this could use a test-case to prevent future regressions.

[dmnks]

There is an argument to be made that the BuildDependency check should also be made when --short-circuit is given (which it is currently not (except for prep after this patch or -bf before) but may be this is a separate issue.

Yeah, sounds like a separate issue to me, if at all. We might as well declare that the prep stage is the first one that cares about BuildRequires so if you skip it, then they won't be checked. Maybe it would deserve a mention in the man page?

So I don't see any other sub commands that should have trouble with the BuildDependency check not being executed properly (assuming the short-circuit issue is ignored). So the patch should actually be sufficient IMHO.

I don't see any other use cases either, so should be fine as it is.

[dmnks]

Yeah, sounds like a separate issue to me, if at all. We might as well declare that the prep stage is the first one that cares about BuildRequires so if you skip it, then they won't be checked. Maybe it would deserve a mention in the man page?

... of course, it still is technically a regression, but if we haven't seen it reported elsewhere so far...

[ffesti]

Added test case and merged the first two patches.

[pmatilai]

Good, one more regression that cannot happen again. Thanks, both of you.