Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 0 - [Medium] Incomplete $(OUTPUT) plumbing in tools/net/ynl/ynltool/Makefile: the patched rule `$(OUTPUT)%.o: %.c`… - [Medium] The `install` target in tools/net/ynl/ynltool/Makefile (lines 49-50) reuses `$(YNLTOOL)` as the destination file name: `$(INSTALL)… -- tools: ynl: Fix out-of-tree build for ynltool This changes the object compile rule in tools/net/ynl/ynltool/Makefile from %.o: %.c to $(OUTPUT)%.o: %.c so that the pattern rule matches the $(OUTPUT)-prefixed object names already used in OBJS. This unblocks make -C tools/net/ynl/ynltool O=<dir>, which previously stopped with "No rule to make target '/tmp/o1/json_writer.o'". Closes: https://lore.kernel.org/all/[email protected]/ > diff --git a/tools/net/ynl/ynltool/Makefile b/tools/net/ynl/ynltool/Makefile > index b8c67cdb4fdf0..1053e7606c86c 100644 > --- a/tools/net/ynl/ynltool/Makefile > +++ b/tools/net/ynl/ynltool/Makefile > @@ -31,7 +31,7 @@ $(YNLTOOL): ../libynl.a $(OBJS) > $(Q)echo -e "\tLINK $@" > $(Q)$(CC) $(CFLAGS) -o $@ $(OBJS) ../libynl.a -lm > > -%.o: %.c ../libynl.a > +$(OUTPUT)%.o: %.c ../libynl.a > $(Q)echo -e "\tCC $@" > $(Q)$(COMPILE.c) -MMD -c -o $@ $< [Severity: Medium] Since -MMD derives the depfile name from the -o argument, does this rule now write the dependency files into $(OUTPUT) as well, i.e. /tmp/o1/foo.d next to /tmp/o1/foo.o? The consumer side of that convention still globs the source directory only: tools/net/ynl/ynltool/Makefile: include $(wildcard *.d) With OUTPUT set to an absolute path ending in '/' (tools/scripts/Makefile.include defines OUTPUT := $(ABSOLUTE_O)/$(if $(subdir),$(subdir)/) when O= is given on the command line), can *.d ever match /tmp/o1/*.d? If it cannot, are header dependencies simply untracked for the O= builds this patch enables? The only remaining prerequisites of $(OUTPUT)%.o are the .c file and ../libynl.a, and that archive has no prerequisites of its own: tools/net/ynl/ynltool/Makefile: ../libynl.a: $(Q)$(MAKE) -C ../ so once ../libynl.a exists make treats it as up to date. Does editing a header under ../lib, or regenerating ../generated, then leave a stale $(OUTPUT)foo.o to be re-linked into $(OUTPUT)ynltool? Related, the clean recipe only touches the source directory: tools/net/ynl/ynltool/Makefile: clean: rm -f *.o *.d *~ Is make O=/tmp/o1 clean a no-op for the artifacts the new rule produces, leaving no way to force the rebuild? Note distclean just below it is $(OUTPUT)-aware through $(YNLTOOL). Would it make sense to change the include to $(wildcard $(OUTPUT)*.d) and the clean recipe to remove $(OUTPUT)*.o and $(OUTPUT)*.d in the same patch? [Severity: Medium] The commit message says "Now, the O= parameter is correctly accounted for", but there is another $(OUTPUT) consumer left in this same Makefile: tools/net/ynl/ynltool/Makefile: YNLTOOL := $(OUTPUT)ynltool ... bindir ?= /usr/bin install: $(YNLTOOL) $(INSTALL) -m 0755 $(YNLTOOL) $(DESTDIR)$(bindir)/$(YNLTOOL) Since $(YNLTOOL) carries the absolute $(OUTPUT) prefix, does make -C tools/net/ynl/ynltool O=/tmp/o1 install expand to install -m 0755 /tmp/o1/ynltool /usr/bin//tmp/o1/ynltool and fail with ENOENT because the intermediate directories do not exist? With a non-empty DESTDIR that happens to contain that nested path, would the binary instead land somewhere unintended? Should the destination be $(DESTDIR)$(bindir)/ynltool, or $(DESTDIR)$(bindir)/$(notdir $(YNLTOOL))? The same path is reachable from the parent Makefile, which recurses with tools/net/ynl/Makefile: @make -C ynltool install and a command-line O= is inherited through MAKEFLAGS, so does make -C tools/net/ynl O=<dir> install fail the same way? Unrelated to O=, bindir ?= /usr/bin hardcodes the path while the prefix ?= /usr declared earlier in the same file is never used, whereas the parent Makefile derives libdir and includedir from $(prefix). Was the inconsistency intended? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910165451.1027541-1-maxime.chevallier%40bootlin.com
