Re: [FFmpeg-devel] [PATCH v2] ffbuild/commonmak: Fix rebuild check with implicit rule chains

[Ramiro Polla <ramiro.polla@gmail.com>]

Hi,

On Sun, May 18, 2025 at 8:30 AM softworkz <ffmpegagent@gmail.com> wrote:
>
> From: softworkz <softworkz@hotmail.com>
>
> When there's a chain of implicit rules, make treats files generated
> inside that chain as intermediate files. Those intermediate files are
> removed after completion of make. When make is run again, it normally
> determines the need for a rebuild by comparing the timestamps of the
> original source file and the final output of the chain and if still
> up-to-date, it doesn't rebuild, even when the intermediate files are
> not present. That makes sense of course - why would it delete them
> otherwise, it would end up in builds being never up-to-date.
> But this original by-the-book logic appeared to be broken and has been
> worked around by adding all intermediate files to the .SECONDARY
> special target, which required extra logic and issues with make clean.
>
> What broke the up-to-date checking is the dependency file generation.
> For the .c files generated by BIN2C the compile target created a .d
> file which indicated that the .ptx.o file has a dependency on the
> ptx.c file. And that dependency broke the normal make behavior with
> intermediate files.
>
> This patch compiles the BIN2C generated .c files without generating
> .d files. In turn the files do not longer need to be added to the
> .SECONDARY target in common.mak.
>
> When interested in those files for debugging, a line can be added to
> the corresponding Makefile like
>
> .SECONDARY: %.ptx.c %.ptx.gz
>
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>     ffbuild/commonmak: Fix rebuild check with implicit rule chains
>
>     When there's a chain of implicit rules, make treats files generated
>     inside that chain as intermediate files. Those intermediate files are
>     removed after completion of make. When make is run again, it normally
>     determines the need for a rebuild by comparing the timestamps of the
>     original source file and the final output of the chain and if still
>     up-to-date, it doesn't rebuild, even when the intermediate files are not
>     present. That makes sense of course - why would it delete them
>     otherwise, it would end up in builds being never up-to-date. But this
>     original by-the-book logic appeared to be broken and has been worked
>     around by adding all intermediate files to the .SECONDARY special
>     target, which required extra logic and issues with make clean.
>
>     What broke the up-to-date checking is the dependency file generation.
>     For the .c files generated by BIN2C the compile target created a .d file
>     which indicated that the .ptx.o file has a dependency on the ptx.c file.
>     And that dependency broke the normal make behavior with intermediate
>     files.
>
>     This patch compiles the BIN2C generated .c files without generating .d
>     files. In turn the files do not longer need to be added to the
>     .SECONDARY target in common.mak.
>
>     When interested in those files for debugging, a line can be added to the
>     corresponding Makefile like
>
>     .SECONDARY: %.ptx.c %.ptx.gz
>
>
>     V2
>     ==
>
>      * Fix MSVC build
>        (use the universal command pattern)
>
> Published-As: https://github.com/ffstaging/FFmpeg/releases/tag/pr-ffstaging-80%2Fsoftworkz%2Fsubmit_commonmak-v2
> Fetch-It-Via: git fetch https://github.com/ffstaging/FFmpeg pr-ffstaging-80/softworkz/submit_commonmak-v2
> Pull-Request: https://github.com/ffstaging/FFmpeg/pull/80
>
> Range-diff vs v1:
>
>  1:  e276f54ffc ! 1:  f468ea2431 ffbuild/commonmak: Fix rebuild check with implicit rule chains
>      @@ ffbuild/common.mak: else
>        endif
>
>       +%.ptx.o: %.ptx.c
>      -+ $(CC) $(CCFLAGS) -x c -c -o $@ $<
>      ++ $(CC) $(CCFLAGS) $(CC_C) $(CC_O) $(patsubst $(SRC_PATH)/%,$(SRC_LINK)/%,$<)
>       +
>       +
>        # 1) Preprocess CSS to a minified version
>      @@ ffbuild/common.mak: else   # NO COMPRESSION
>        endif
>
>       +%.html.o: %.html.c
>      -+ $(CC) $(CCFLAGS) -x c -c -o $@ $<
>      ++ $(CC) $(CCFLAGS) $(CC_C) $(CC_O) $(patsubst $(SRC_PATH)/%,$(SRC_LINK)/%,$<)
>       +
>       +%.css.o: %.css.c
>      -+ $(CC) $(CCFLAGS) -x c -c -o $@ $<
>      ++ $(CC) $(CCFLAGS) $(CC_C) $(CC_O) $(patsubst $(SRC_PATH)/%,$(SRC_LINK)/%,$<)
>       +
>       +
>        clean::
>
>
>  ffbuild/common.mak | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/ffbuild/common.mak b/ffbuild/common.mak
> index 0e1eb1f62b..d9462271d5 100644
> --- a/ffbuild/common.mak
> +++ b/ffbuild/common.mak
> @@ -139,6 +139,10 @@ else
>         $(BIN2C) $(patsubst $(SRC_PATH)/%,$(SRC_LINK)/%,$<) $@ $(subst .,_,$(basename $(notdir $@)))
>  endif
>
> +%.ptx.o: %.ptx.c
> +       $(CC) $(CCFLAGS) $(CC_C) $(CC_O) $(patsubst $(SRC_PATH)/%,$(SRC_LINK)/%,$<)
> +
> +
>  # 1) Preprocess CSS to a minified version
>  %.css.min: %.css
>         # Must start with a tab in the real Makefile
> @@ -177,6 +181,13 @@ else   # NO COMPRESSION
>         $(BIN2C) $< $@ $(subst .,_,$(basename $(notdir $@)))
>  endif
>
> +%.html.o: %.html.c
> +       $(CC) $(CCFLAGS) $(CC_C) $(CC_O) $(patsubst $(SRC_PATH)/%,$(SRC_LINK)/%,$<)
> +
> +%.css.o: %.css.c
> +       $(CC) $(CCFLAGS) $(CC_C) $(CC_O) $(patsubst $(SRC_PATH)/%,$(SRC_LINK)/%,$<)
> +

I'm not a big fan of adding new targets with a stripped-down version
of the original rule. It seems like unnecessary duplication, and
increases the chance of divergence over time.

Instead, I think it's cleaner to use target-specific variables to
disable a part of the original rule. For example, in this case, you
would disable the dependency generation for just those targets, and
end up with something like this:
%.html.o: CC_DEPFLAGS =
%.css.o: CC_DEPFLAGS =

Ramiro
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".