| Message ID | 20210223114132.24345-7-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: add {program,xdiff,test,git}-objs & objects targets | expand |
On Tue, Feb 23, 2021 at 12:41:32PM +0100, Ævar Arnfjörð Bjarmason wrote: > Adding $(FUZZ_OBJS) as a dependency on "all" was intentionally done in > 5e472150800 (fuzz: add basic fuzz testing target., 2018-10-12). > > Rather than needlessly build these objects which aren't required for > the build every time we make "all", let's instead move them to be > built by the CI jobs. > > The goal is to make sure that we don't inadvertently break these, we > can accomplish that goal by building them in CI, rather than slowing > down every build of git for everyone everywhere. The current state is that regular devs are responsible for avoiding compile breakages in the fuzz objects, even if they don't care themselves. Your earlier patches turned this into: regular devs are not on the hook for breaking fuzz objects; they are the responsibility of fuzz people. I'm OK with either of those, but this approach seems to me like the worst of both worlds. ;) If you do a refactor, you are still on the hook for breaking the fuzz objects because CI will fail (and you have to investigate it, and fix it for CI to remain a useful tool). But instead of finding out about the problem quickly as you're working, instead you push up what you think is a finished result, and then from minutes to hours later you get a notification telling you that oops, you missed a spot. I find that the shorter the error-fix-compile cycle is, the less time I waste waiting or context-switching. If we had a ton of fuzz object files that took forever to build, the savings on each build might be worth it. But AFAICT (from timing "make clean; make -j1" before and after), we are saving less than 1% of the build time (which is way less than the run-to-run noise). It doesn't seem like the right tradeoff to me. (Likewise, if other CI-only checks we have, like coccinelle, could be run at a similar cost, I'd recommend sticking them into the default developer build). One thing we _could_ do is stop building fuzz objects as part of "all", but include them for DEVELOPER=1 builds (which includes CI). That keeps them from hurting normal users (who don't actually need them), but prevents bitrot. It doesn't address your original motivation though (you as a developer would probably still be building them). -Peff
Jeff King <peff@peff.net> writes: > If you do a refactor, you are still on the hook for breaking the fuzz > objects because CI will fail (and you have to investigate it, and fix it > for CI to remain a useful tool). But instead of finding out about the > problem quickly as you're working, instead you push up what you think is > a finished result, and then from minutes to hours later you get a > notification telling you that oops, you missed a spot. I find that the > shorter the error-fix-compile cycle is, the less time I waste waiting or > context-switching. Thanks for writing this down so clearly.
On Tue, Feb 23 2021, Jeff King wrote: > On Tue, Feb 23, 2021 at 12:41:32PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> Adding $(FUZZ_OBJS) as a dependency on "all" was intentionally done in >> 5e472150800 (fuzz: add basic fuzz testing target., 2018-10-12). >> >> Rather than needlessly build these objects which aren't required for >> the build every time we make "all", let's instead move them to be >> built by the CI jobs. >> >> The goal is to make sure that we don't inadvertently break these, we >> can accomplish that goal by building them in CI, rather than slowing >> down every build of git for everyone everywhere. > > The current state is that regular devs are responsible for avoiding > compile breakages in the fuzz objects, even if they don't care > themselves. Your earlier patches turned this into: regular devs are not > on the hook for breaking fuzz objects; they are the responsibility of > fuzz people. I'm OK with either of those, but this approach seems to me > like the worst of both worlds. ;) > > If you do a refactor, you are still on the hook for breaking the fuzz > objects because CI will fail (and you have to investigate it, and fix it > for CI to remain a useful tool). But instead of finding out about the > problem quickly as you're working, instead you push up what you think is > a finished result, and then from minutes to hours later you get a > notification telling you that oops, you missed a spot. I find that the > shorter the error-fix-compile cycle is, the less time I waste waiting or > context-switching. > > If we had a ton of fuzz object files that took forever to build, the > savings on each build might be worth it. But AFAICT (from timing "make > clean; make -j1" before and after), we are saving less than 1% of the > build time (which is way less than the run-to-run noise). > > It doesn't seem like the right tradeoff to me. (Likewise, if other > CI-only checks we have, like coccinelle, could be run at a similar cost, > I'd recommend sticking them into the default developer build). It's mainly psychological and doesn't contribute much to overall build time as a percentage, but I find it grating that the last thing I see before I switch away from that terminal when firing off a build on a slower GCC farm box I can only use -j1 on, is these fuzz objects taking 2-3 seconds to build, knowing I'm wasting time on something I'll never need. I think when we build something we should narrowly be compiling only the things we need, not running some sort of pseudo-CI on every developer's computer. We can have CI or other targets for that. Besides, if we were going for some sane cost-benefit here we'd have targets to try compiling with NO_CURL=1 or some other conditional setups that are actually common in the wild. > One thing we _could_ do is stop building fuzz objects as part of "all", > but include them for DEVELOPER=1 builds (which includes CI). That keeps > them from hurting normal users (who don't actually need them), but > prevents bitrot. It doesn't address your original motivation though (you > as a developer would probably still be building them). Please no. A very good thing about how DEVELOPER=1 works is that we're not doing anything extra except advisory compilation flags. It's turned on for "production" builds in a lot of settings because of that. It would also be very annoying to e.g. have some failure on Solaris or whatever, debug it with DEVELOPER=1, and then get some completely unrelated failure in the developer-only code, e.g. because we'd decided to compile all of fuzz/NO_OPENSSL/NO_CURL etc. and had some bug there. Yes that bug would be worthwhile to fix, but not right there and then. So having it under some "make all-combinations" flag or whatever would be better.
On Sun, Feb 28, 2021 at 09:13:54PM +0100, Ævar Arnfjörð Bjarmason wrote: > > The current state is that regular devs are responsible for avoiding > > compile breakages in the fuzz objects, even if they don't care > > themselves. Your earlier patches turned this into: regular devs are not > > on the hook for breaking fuzz objects; they are the responsibility of > > fuzz people. I'm OK with either of those, but this approach seems to me > > like the worst of both worlds. ;) > > > > If you do a refactor, you are still on the hook for breaking the fuzz > > objects because CI will fail (and you have to investigate it, and fix it > > for CI to remain a useful tool). But instead of finding out about the > > problem quickly as you're working, instead you push up what you think is > > a finished result, and then from minutes to hours later you get a > > notification telling you that oops, you missed a spot. I find that the > > shorter the error-fix-compile cycle is, the less time I waste waiting or > > context-switching. > > > > If we had a ton of fuzz object files that took forever to build, the > > savings on each build might be worth it. But AFAICT (from timing "make > > clean; make -j1" before and after), we are saving less than 1% of the > > build time (which is way less than the run-to-run noise). > > > > It doesn't seem like the right tradeoff to me. (Likewise, if other > > CI-only checks we have, like coccinelle, could be run at a similar cost, > > I'd recommend sticking them into the default developer build). > > It's mainly psychological and doesn't contribute much to overall build > time as a percentage, but I find it grating that the last thing I see > before I switch away from that terminal when firing off a build on a > slower GCC farm box I can only use -j1 on, is these fuzz objects taking > 2-3 seconds to build, knowing I'm wasting time on something I'll never > need. Sure, I find it annoying, too. And I am totally fine with saying "nope, let them bitrot if nobody cares enough about them to build". That is after all what happens with a bunch of stuff in compat/, or custom code like NO_PTHREADS, or with/without pcre, curl, etc. This just seems like a bad middle ground. > I think when we build something we should narrowly be compiling only the > things we need, not running some sort of pseudo-CI on every developer's > computer. We can have CI or other targets for that. > > Besides, if we were going for some sane cost-benefit here we'd have > targets to try compiling with NO_CURL=1 or some other conditional setups > that are actually common in the wild. Right. So that mostly just argues to me for not compiling them ever unless they are needed. I.e., dropping the CI part of your patch. > > One thing we _could_ do is stop building fuzz objects as part of "all", > > but include them for DEVELOPER=1 builds (which includes CI). That keeps > > them from hurting normal users (who don't actually need them), but > > prevents bitrot. It doesn't address your original motivation though (you > > as a developer would probably still be building them). > > Please no. A very good thing about how DEVELOPER=1 works is that we're > not doing anything extra except advisory compilation flags. It's turned > on for "production" builds in a lot of settings because of that. I'm not convinced that we should limit the DEVELOPER flag for this reason in general. The point of the flag is to add extra linting, including stopping the build if need be. If that is in tension with somebody using it for production builds, I would always choose improving the developer experience. That said, I don't at all care about linting the fuzz code, so I don't think it's a very compelling case. -Peff
diff --git a/Makefile b/Makefile index 10f7baa74b0..cfad590a194 100644 --- a/Makefile +++ b/Makefile @@ -665,9 +665,6 @@ FUZZ_OBJS += fuzz-pack-idx.o .PHONY: fuzz-objs fuzz-objs: $(FUZZ_OBJS) -# Always build fuzz objects even if not testing, to prevent bit-rot. -all:: $(FUZZ_OBJS) - FUZZ_PROGRAMS += $(patsubst %.o,%,$(FUZZ_OBJS)) # Empty... @@ -3322,8 +3319,8 @@ FUZZ_CXXFLAGS ?= $(CFLAGS) .PHONY: fuzz-all -$(FUZZ_PROGRAMS): all +$(FUZZ_PROGRAMS): all fuzz-objs $(QUIET_LINK)$(CXX) $(FUZZ_CXXFLAGS) $(LIB_OBJS) $(BUILTIN_OBJS) \ $(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@ -fuzz-all: $(FUZZ_PROGRAMS) +fuzz-all: $(FUZZ_PROGRAMS) $(FUZZ_OBJS) diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index a66b5e8c75a..85d260476c5 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -10,6 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; *) ln -s "$cache_dir/.prove" t/.prove;; esac +make fuzz-objs make case "$jobname" in linux-gcc)
Adding $(FUZZ_OBJS) as a dependency on "all" was intentionally done in 5e472150800 (fuzz: add basic fuzz testing target., 2018-10-12). Rather than needlessly build these objects which aren't required for the build every time we make "all", let's instead move them to be built by the CI jobs. The goal is to make sure that we don't inadvertently break these, we can accomplish that goal by building them in CI, rather than slowing down every build of git for everyone everywhere. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 7 ++----- ci/run-build-and-tests.sh | 1 + 2 files changed, 3 insertions(+), 5 deletions(-)