Re: [PATCH v2] Makefile: libfdt: build only the strict necessary

[Philippe Mathieu-Daudé]

On 4/9/20 6:33 PM, Philippe Mathieu-Daudé wrote:
> Hi Claudio,
>
> On 4/9/20 2:43 PM, Claudio Fontana wrote:
> > when building dtc/libfdt, we were previously using dtc/Makefile,
> > which tries to build some artifacts that are not needed,
> > and can complain on stderr about the absence of tools that
> > are not required to build just libfdt.
> >
> > Instead, build only the strict necessary to get libfdt.a .
> >
> > Remove the subdir-dtc "compatibility gunk" for recursion,
> > since we are not recursing anymore.
> >
> > Signed-off-by: Claudio Fontana <address@hidden>
> > ---
> >   Makefile  | 23 +++++++++++++----------
> >   configure |  6 +-----
> >   rules.mak |  2 ++
> >   3 files changed, 16 insertions(+), 15 deletions(-)
> >
> > v1 -> v2:
> >
> > * fix error generated when running UNCHECKED_GOALS without prior 
> > configure,
> >    for example during make docker-image-fedora. Without configure, 
> > DSOSUF is
> >    empty, and the module pattern rule in rules.mak that uses this 
> > variable
> >    can match too much; provide a default in the Makefile to avoid it.
> >
> > * only attempt to build the archive when there is a non-empty list of 
> > objects.
> >    This could be done in general for the %.a: pattern in rules.mak, 
> > but maybe
> >    there are valid reasons to build an empty .a?
> >
> > * removed some intermediate variables that did not add much value
> >    (LIBFDT_srcdir, LIBFDT_archive)
> >
> > Tested locally with 3 VPATH configurations (no-, VPATH, VPATH in src 
> > subdir),
> > and with docker-image-fedora, docker-test-debug@fedora that failed 
> > before.
> >
> > diff --git a/Makefile b/Makefile
> > index 84ef881600..92bc853b5f 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -4,6 +4,10 @@ ifneq ($(words $(subst :, ,$(CURDIR))), 1)
> >     $(error main directory cannot contain spaces nor colons)
> >   endif
> > +# some pattern rules in rules.mak are confused by an empty DSOSUF,
> > +# and UNCHECKED_GOALS for testing (docker-) can run without prior 
> > configure.
> > +DSOSUF ?= ".so"
> > +
> >   # Always point to the root of the build tree (needs GNU make).
> >   BUILD_DIR=$(CURDIR)
> > @@ -526,15 +530,16 @@ $(SOFTMMU_FUZZ_RULES): $(edk2-decompressed)
> >   $(TARGET_DIRS_RULES):
> >       $(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $(dir $@) 
> > V="$(V)" TARGET_DIR="$(dir $@)" $(notdir $@),)
> > -DTC_MAKE_ARGS=-I$(SRC_PATH)/dtc VPATH=$(SRC_PATH)/dtc -C dtc V="$(V)" 
> > LIBFDT_srcdir=$(SRC_PATH)/dtc/libfdt
> > -DTC_CFLAGS=$(CFLAGS) $(QEMU_CFLAGS)
> > -DTC_CPPFLAGS=-I$(BUILD_DIR)/dtc -I$(SRC_PATH)/dtc 
> > -I$(SRC_PATH)/dtc/libfdt
> > -
> > -.PHONY: dtc/all
> > -dtc/all: .git-submodule-status dtc/libfdt dtc/tests
>
> I'm getting:
>
> config-host.mak is out-of-date, running configure
> make: *** No rule to make target 'dtc/all', needed by 'config-host.h'. 
> Stop.
>
> On second try it works.

FYI same happens when going back (previous this patch applied) but there 
is nothing we can do to prevent that afaik:

config-host.mak is out-of-date, running configure
make: *** No rule to make target 'libfdt', needed by 'config-host.h'.  Stop.

> Instead of alarming users, we could keep this target as a silent no-op, 
> then remove it after some time.
>
> For the rest, patch looks good, nice cleanup!
>
> Regards,
>
> Phil.
>
> > -    $(call quiet-command,$(MAKE) $(DTC_MAKE_ARGS) 
> > CPPFLAGS="$(DTC_CPPFLAGS)" CFLAGS="$(DTC_CFLAGS)" 
> > LDFLAGS="$(QEMU_LDFLAGS)" ARFLAGS="$(ARFLAGS)" CC="$(CC)" AR="$(AR)" 
> > LD="$(LD)" $(SUBDIR_MAKEFLAGS) libfdt/libfdt.a,)
> > +LIBFDT_objdir = dtc/libfdt
> > +-include $(SRC_PATH)/dtc/libfdt/Makefile.libfdt
> > +LIBFDT_objects = $(addprefix $(LIBFDT_objdir)/, $(LIBFDT_OBJS))
> > +.PHONY: libfdt
> > +libfdt: .git-submodule-status $(LIBFDT_objdir)/libfdt.a
> > +$(LIBFDT_objdir)/libfdt.a: $(LIBFDT_objects)
> > +    $(if $(LIBFDT_objects),$(call quiet-command,rm -f $@ && $(AR) rcs 
> > $@ $^,"AR","$(TARGET_DIR)$@"),)
> > -dtc/%: .git-submodule-status
> > +$(LIBFDT_objects): | $(LIBFDT_objdir)
> > +$(LIBFDT_objdir): .git-submodule-status
> >       @mkdir -p $@
> >   # Overriding CFLAGS causes us to lose defines added in the 
> > sub-makefile.
> > @@ -563,7 +568,6 @@ slirp/all: .git-submodule-status
> >   # Compatibility gunk to keep make working across the rename of targets
> >   # for recursion, to be removed some time after 4.1.
> > -subdir-dtc: dtc/all
> >   subdir-capstone: capstone/all
> >   subdir-slirp: slirp/all
> > @@ -821,7 +825,6 @@ distclean: clean
> >       rm -rf $$d || exit 1 ; \
> >           done
> >       rm -Rf .sdk
> > -    if test -f dtc/version_gen.h; then $(MAKE) $(DTC_MAKE_ARGS) 
> > clean; fi
> >   KEYMAPS=da     en-gb  et  fr     fr-ch  is  lt  no  pt-br  sv \
> >   ar      de     en-us  fi  fr-be  hr     it  lv  nl         pl  
> > ru     th \
> > diff --git a/configure b/configure
> > index 233c671aaa..36f83ffc5a 100755
> > --- a/configure
> > +++ b/configure
> > @@ -4278,10 +4278,6 @@ EOF
> >         if test -d "${source_path}/dtc/libfdt" || test -e 
> > "${source_path}/.git" ; then
> >             fdt=git
> >             mkdir -p dtc
> > -          if [ "$pwd_is_source_path" != "y" ] ; then
> > -              symlink "$source_path/dtc/Makefile" "dtc/Makefile"
> > -              symlink "$source_path/dtc/scripts" "dtc/scripts"
> > -          fi
> >             fdt_cflags="-I\$(SRC_PATH)/dtc/libfdt"
> >             fdt_ldflags="-L\$(BUILD_DIR)/dtc/libfdt"
> >             fdt_libs="$fdt_libs"
> > @@ -8151,7 +8147,7 @@ echo "PIXMAN_CFLAGS=$pixman_cflags" >> 
> > $config_host_mak
> >   echo "PIXMAN_LIBS=$pixman_libs" >> $config_host_mak
> >   if [ "$fdt" = "git" ]; then
> > -  echo "config-host.h: dtc/all" >> $config_host_mak
> > +  echo "config-host.h: libfdt" >> $config_host_mak
> >   fi
> >   if [ "$capstone" = "git" -o "$capstone" = "internal" ]; then
> >     echo "config-host.h: capstone/all" >> $config_host_mak
> > diff --git a/rules.mak b/rules.mak
> > index 694865b63e..61eb474ba4 100644
> > --- a/rules.mak
> > +++ b/rules.mak
> > @@ -105,6 +105,8 @@ LINK = $(call quiet-command, $(LINKPROG) $(CFLAGS) 
> > $(QEMU_LDFLAGS) -o $@ \
> >   DSO_OBJ_CFLAGS := -fPIC -DBUILD_DSO
> >   module-common.o: CFLAGS += $(DSO_OBJ_CFLAGS)
> > +
> > +# Note: DSOSUF must not be empty, or these rules will try to match 
> > too much
> >   %$(DSOSUF): QEMU_LDFLAGS += $(LDFLAGS_SHARED)
> >   %$(DSOSUF): %.mo
> >       $(call LINK,$^)