Hi Tom,

The change itself seems reasonable enough. I think the comment explaining that this allows redefining %__make to adjust the make arguments should be in the commit message. It might even be expanded a bit to say why that would be useful for the test suite.

(Nothing gets compiled in %check, so I am a little curious what options might be useful to add here. I'm assuming you noticed this while working on rebuilding the distro with clang. Maybe that's a bad assumption. Or this is just a belt & suspenders change to ensure that we use %__make consistently, in case something ever does get compiled from the test suite.)

Thanks!

Hi,

I am working on rebuilding packages with clang the issue here is that the make test target uses gcc to compile fuzz-commit-graph.c, so I need to be able to inject the CC=clang argument to make in order for it to use clang.

I've updated the pull request with a more descriptive commit message.

Interesting. In the logs from my recent koji builds, I see fuzz-commit-graph.c compiled in %build, as it's part of the default make target. Does that differ in your builds? I'm curious if there's something which could possibly be improved upstream or elsewhere in the git spec file.

Thinking about it, it may well be because %build uses %make_build and your test builds for clang adjust the make flags, which triggers the later plain make test to see the build flags have changed and causes recompilation of fuzz-commit-graph. That would be very good reason to fix this, as it will prevent any other needless recompilation -- which I've just been lucky enough not to run into.

If you've got a build log, I'd be curious to check it out and see if that's the case.

Thanks. (Hopefully this isn't too pedantic -- I just like to know why things happen so we can record them for down the road. That way someone else will know why we changed things.)

Here is the build log: https://tstellar.fedorapeople.org/git-2.25.0-1.log

One thing I should mention is that the %make_build macro also adds _smp_flags (to enable parallel make jobs and also the -O output sync flag, which buffers output for each make target and only displays it when the target is finished. If you don't want these extra features, the other option would be to use the %__make macro instead of %make_build.

Thanks. (Hopefully this isn't too pedantic -- I just like to know why things happen so we can record them for down the road. That way someone else will know why we changed things.)

It's not. This is great feedback and will help me as I fix other packages.

Cool, thanks for the build log. It is indeed git's build noticing the changed flags. And the fact that it then fails to compile and bails out of the build is certainly a road block to your work on clang. I didn't realize it broke the build. I guess that made it pretty easy to notice that something needed fixed here. :)

Thanks for mentioning and thinking about the -O and %_smp_flags options. I used to avoid using %_smp_flags when I was running the test suite without the perl-based "prove" test harness. Once we moved to that we get the parallelism from "prove" via GIT_PROVE_OPTS which includes %_smp_flags. So while I don't think we'll benefit much from having them in the make call, it also doesn't hurt AFAICT. I ran a scratch build with your changes last night and it went fine on fedora and the epel targets.

If we notice later that having -O in the make test flags is annoying while running a package build interactively we can change it. But I rarely run it that way. If I am debugging the test suite interactively, I just run the specific test(s) directly.

Thanks for catching this and helping me understand it better. And thanks for the work you're doing to build with clang. That'll be interesting to follow and will surely lead to some nice improvements across the entire Fedora package set.