Make sure to install BuildRequires defined by macros

[FrostyX]

Fix #1652

[gemini-code-assist]

Code Review

This pull request correctly addresses an issue where BuildRequires defined by macros were not being installed before the SRPM rebuild, potentially causing the rebuild to fail. The change moves the dependency installation to an earlier stage, which is the right approach. My review includes one suggestion to improve the code's robustness by using a dedicated helper function for path construction.

[FrostyX]

I updated the PR so that it doesn't install the dependencies based on the copied-in original SRPM but so that we rather rebuild the SRPM twice as @praiskup suggested. This seems to work too.

I wanted to repeat the loop again and again as long as it still installs new dependencies but both self.install_external(requires) and self.installSrpmDeps(rebuilt_srpm) return None so not possible. Unless we change them too.

[praiskup]

This is a great start, thank you!

Even with dynamic deps, we do not check for return value, but compare lists of installed packages before/after:

mock/mock/py/mockbuild/backend.py

Line 771 in 194159e

if packages_after == packages_before:

Do you think it makes sense in this case? If yes, I'd prefer having a new configuration option to stop the loop at some reasonable level.

[FrostyX]

Ah, really cool, thank you! I added the same condition.

I'd prefer having a new configuration option to stop the loop at some reasonable level.

Do you? I can definitely add a configuration option for that, it's not a problem. To me, it doesn't seem like an option you would want to play with. Your call though, its fine by me either way.

[praiskup]

Heh, yes, I think we should have the config option. It's absolutely good to have an option to play with :-) but I like consistency, and the loop for dynamic deps also has its config. And considering this change may have side effects, and cause some build failures, people might want to have an option to turn this looping off (loops = 1)?

[praiskup]

Would you mind merging #1661 and rebasing on top of it to have all the checks green?

[praiskup]

Tested & works. Thank you. Also with centos-6-x86_64. Tested example:

BuildRequires: tcsh
%if %(test -f /bin/tcsh; echo $?) == 0
BuildRequires: star
%endif

[FrostyX]

Would you mind merging #1661 and rebasing on top of it to have all the checks green?

Sure, I will wait until it has a green check mark and then merge.

Heh, yes, I think we should have the config option.

Do we want to reuse the self.config.get('dynamic_buildrequires_max_loops') or do we want to have a separate one (e.g. macro_buildrequires_max_loops)?

[praiskup]

Preferably, it would be a separate option; consider somebody wants to disable the new looping mechanism (and leave the dynamic deps looping alone). What if we used static_? Macros are typically included in both, synamic and normal (static?) buildrequires.

[FrostyX]

Updated, PTAL.

[praiskup]

Very nice, thank you @FrostyX !