[FFmpeg-devel] [PATCH v2] ffbuild/commonmak: Fix rebuild check with implicit rule chains
softworkz .
softworkz at hotmail.com
Wed May 21 00:51:39 EEST 2025
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of softworkz .
> Sent: Dienstag, 20. Mai 2025 23:13
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2] ffbuild/commonmak: Fix rebuild check
> with implicit rule chains
>
>
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Ramiro
> Polla
> > Sent: Dienstag, 20. Mai 2025 22:29
> > To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH v2] ffbuild/commonmak: Fix rebuild check
> > with implicit rule chains
> >
> > On Tue, May 20, 2025 at 9:46 PM softworkz .
> > <softworkz-at-hotmail.com at ffmpeg.org> wrote:
> > > > -----Original Message-----
> > > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Ramiro
> > Polla
> > > > Sent: Dienstag, 20. Mai 2025 21:36
> > > > To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> > > > Subject: Re: [FFmpeg-devel] [PATCH v2] ffbuild/commonmak: Fix rebuild
> > check
> > > > with implicit rule chains
> > > > On Sun, May 18, 2025 at 8:30 AM softworkz <ffmpegagent at gmail.com> wrote:
> > > > > From: softworkz <softworkz at 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 at 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 =
> > >
> > > Hi Ramiro,
> > >
> > > I didn't know that one can do that. That's clearly a better way, then.
> > >
> > > What about the first line in the COMPILE macro:
> > >
> > > define COMPILE
> > > $(call $(1)DEP,$(1))
> > > $($(1)) $($(1)FLAGS) $($(2)) $($(1)_DEPFLAGS) $($(1)_C) $($(1)_O)
> > $(patsubst $(SRC_PATH)/%,$(SRC_LINK)/%,$<)
> > > endef
> > >
> > > I haven't traced back what it actually does - is it harmless when
> > > it's executed in those cases where no dependency file is desired?
> > >
> > > If that's fine, I'd send an updated patch with your suggestion.
> >
> > I had forgotten about that part. It's not triggered with gcc/clang,
> > but it seems to be triggered when using msvc. I don't have an msvc
> > setup ready, but I believe you do, right?
>
> My local MSVC setup is using vcxproj projects, it's not makefile driven,
> but I have the other ways as well. Though, I love to have it off the table
> and running elsewhere. I made the V=1 change and also added a second
> make after make to check:
>
> https://dev.azure.com/githubsync/ffmpeg/_build/results?buildId=89332&view=logs
>
>
> BTW, it's public, just create a PR on https://github.com/ffstaging/FFmpeg
> and you get 6 CI builds which are the same like on Patchwork.
> (nobody needs to use that submission thing, just close the PR right after)
>
>
> Best,
> sw
> _______________________________________________
Ah, it seems to have created the .d files but doesn't take them into account
when rebuilding. So the first line creates the .d files in case of MSVC.
I'm not quite sure how this first line could be eliminated. When clearing
CCDEP, there would still be the "call" remaining?
Here are the last few lines. I've attached a larger snippet because some
of the lines are > 800 chars long.
sw
-----
rm -f ffmpeg.exe
cp -p ffprobe_g.exe ffprobe.exe
cp -p ffmpeg_g.exe ffmpeg.exe
echo skipping strip ffprobe.exe
echo skipping strip ffmpeg.exe
skipping strip ffprobe.exe
skipping strip ffmpeg.exe
rm fftools/resources/graph.css.c fftools/resources/graph.css.min fftools/resources/graph.html.c
make: Nothing to be done for 'all'.
Finishing: Make
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: msvc_log.txt
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250520/88cd3d6c/attachment.txt>
More information about the ffmpeg-devel
mailing list