mock should fail build early when dynamic build requires are not satisfied

[praiskup]

Commit ba5e4ef added a hack
which causes that even if rpmbuild returns 11 (some dynamic buildrequires
are missing) mock continues as if rpm succeeded. This has negative that
the build can fail a very late during the %build process.

The problem is that the current released rpm with dynamic BR
support is broken and we can not drop the hack from mock; at
least not till we can expect broken RPM. @hrnciar please link the
RPM bug once it is filled.

[praiskup]

We should at least throw a huge warning that we continue to %build even though
we shouldn't. Perhaps we could detect whether RPM is fixed... and stop doing the
hack in such case.

[hrnciar]

rpm-software-management/rpm#963

[ignatenkobrain]

I don't see what is the hack in mentioned comment. You can have multiple levels of dynamic BRs, so you want to iterate through generating them over and over until all fail or all get installed.

[praiskup]

+                   if packages_after == packages_before:
+                        success = True

This is the hack I had in mind. Do you think it is correct, ie that we should ignore rpm
retval?

[ignatenkobrain]

This is definitely not a hack. The problem is that mock uses --nodeps, so the rpmbuild is not sure whether all dependencies are satisfied or not. Obviously it can exit with 1, but that would indicate that nosrc.rpm would not be written.

[praiskup]

I don't get it. Can you interpret what the exit status 11 means, then?

[ignatenkobrain]

that buildreqs.nosrc.rpm has been generated

[praiskup]

Is this documented somewhere? I read RPMRC_MISSINGBUILDREQUIRES.

[ignatenkobrain]

I'm not sure if it is documented, however if you pass --nodeps you can't ever have MISSINGBUILDREQUIRES, because you omit checking it. And the only way RPM can tell you this is to return 11 always and you will have to try installing Requires of nosrc.rpm and checking whether something was installed or not (or fail).

[hrnciar]

@pmatilai could you please take a look?
Is it really correct to return 11 even though all dependencies were satisfied?

[hrnciar]

@pmatilai ping

[pmatilai]

You're asking the wrong guy, dynamic buildrequires were done by @ffesti and @ignatenkobrain. If @ignatenkobrain says it's working as designed, I guess it is.

[praiskup]

@ignatenkobrain says it's working as designed, I guess it is.

This isn't question about implementation of dynamic requires, but somewhat
broader question about the rpmbuild design:

• what exit status 11 means, and
• is it correct to return 11 when no BuildRequires is
  are missed?

Depending on definitive answer to those two, we need to think about either
fixing the current status, or adding a new option that won't force us to ...

@ignatenkobrain wrote:

you will have to try installing Requires of nosrc.rpm and checking
whether something was installed or not (or fail).

... re-try the re-installation of the nosrc.rpm, it doesn't make sense
because that only may lead to:

• expensive no-op dnf install operation, and
• terribly ugly check whether something was really installed in the very
  last dnf transaction

We need to resolve this somehow to make the overall implementation of
dynamic buildrequires feature 100% clear.

[ignatenkobrain]

is it correct to return 11 when no BuildRequires is are missed?

rpmbuild does not know that because it has been told to use --nodeps.

expensive no-op dnf install operation, and
terribly ugly check whether something was really installed in the very last dnf transaction

You can check if all dependencies with rpm without involving dnf, but it was easiest way for me to do it using dnf in mock.

[praiskup]

rpmbuild does not know that because it has been told to use --nodeps.

Maybe we should have a way to do the task without passing --nodeps
then? Or have some special option which will give us exit_status 0
when we don't have to re-try.. dunno.

You can check if all dependencies with rpm without involving dnf,

The point is to not even involve rpm, it's sub-optimal ... it would be much
better if rpm-build told us not to do anything else at all.

[dmnks]

Here's the relevant RPM commit that introduced dynamic BuildRequires:
rpm-software-management/rpm@58dcfdd

The reason for rpmbuild returning 11 is that, when processing the dynamic build deps in the spec file, we would set the return code to RPMBUILD_MISSINGBUILDREQUIRES (11) early on and then, if --nodeps is used, skip the subsequent dependency check that would otherwise reset the return code depending on the outcome. That makes the rest of the function think that we failed satisfying the deps, and eventually return with the RPMBUILD_MISSINGBUILDREQUIRES code.

Unless this behavior is intentional, I think we should go ahead and fix it in RPM. @ffesti, @ignatenkobrain, what's your opinion?

[praiskup]

Meh, I see the problem here now, ... we need --nodeps because we can not
rely on fact that rpmbuild inside chroot can read rpm database (it is generated
from the outside, by host's rpm).

We could rely on that if we used --bootstrap-chroot, but it wouldn't be OK to
require --bootstrap-chroot for dynamic build requires.

[ignatenkobrain]

As I said many times, this behavior is intentional. Either stop using --nodeps (which is not desirable from what I know) or live with this :)

[dmnks]

@ignatenkobrain What's the rationale behind returning an error code if the user has expressed his desire to not run a dependency check? (just curious)

[ignatenkobrain]

@dmnks how else user will be notified that nosrc.rpm is generated and he should explicitly check its dependencies?

[praiskup]

I tested -br without --nodeps, and indeed 11 is only returned when
some (dynamic) buildrequires are missing. 0 is returned if both
{dynamic,}buildrequires are satisfied. So, @ignatenkobrain was right,
there's nothing RPM can do.

In mock though, we could at least avoid doing the last dnf install and
save some cpu cycles, .. and some network traffic -- we can remember the
set() of *nosrc* Requires:`, and try to install them only if some of
them are new.

Perhaps - while we are on that - we could also throw a warning if the new
set of build requires isn't superset of the previous run (or even fail,
because that would eventually lead to incomplete set of Requires:
generated in the final *.src.rpm).

[ignatenkobrain]

In mock though, we could at least avoid doing the last dnf install and
save some cpu cycles, .. and some network traffic -- we can remember the
set() of nosrc Requires:`, and try to install them only if some of
them are new.

Yes, you should use RPM to check if the dependencies are satisfied. However, as I said earlier, it was not trivial to do in the mock codebase.

Perhaps - while we are on that - we could also throw a warning if the new
set of build requires isn't superset of the previous run (or even fail,
because that would eventually lead to incomplete set of Requires:
generated in the final *.src.rpm).

I'm not sure if we need that. But up to you :)

[dmnks]

@ignatenkobrain What I mean is, when I use --nodeps and some of the deps are not satisfied, why should I receive an error anyway? By passing --nodeps I explicitly said "I don't care about dependencies at this time". (I'm not saying the current behavior is wrong, though. I'm just trying to understand) :)

[praiskup]

@dmnks indeed, it's matter of taste -- we use exit status 11 as indication that
nosrc.rpm was generated; but we could as well detect that by fact that nosrc
exists. More, it IMO isn't correct to use exit status 11 there, there should be
special exit status for this situation.

[dmnks]

@praiskup OK, I see. Sure, if it works for you this way, then there's no point in changing anything, since nobody else has complained about this particular behavior (and the consensus here seems to be that it's correct).

[praiskup]

Heh, we should actually fix mock to expect exit_status 0 as well as 11 in
this case (currently, if RPM returned 0, mock would behave unexpectedly).

By doing -br --nodeps we actually know in advance the nosrc.rpm file
will be generated. No need to check for exit status 11/0.

nobody else has complained about this particular behavior (and the consensus here seems to be that it's correct)

So... I eventually think that it is semantically incorrect. That
said, unless there's any other error - rpm should return 0 with
--nodeps. And mock should be fixed.

[dmnks]

So... I eventually think that it is semantically incorrect. That
said, unless there's any other error - rpm should return 0 with
--nodeps. And mock should be fixed.

OK thanks for the feedback. It does indeed make sense to me, too (to return 0 with --nodeps unless there are other errors), which is why I pointed this out in the beginning.

[praiskup]

Before you start with RPM, can we please fix mock first, so we are 100% sure
that our assumption is correct?

[dmnks]

@praiskup Yup, thanks, I just noted that in our issue as well:
rpm-software-management/rpm#963 (comment)