Re: [PATCH] Makefile: avoid undefined variables

[Martin D Kealey]

On Fri, 25 Oct 2024 at 05:07, Grisha Levit <grishalevit@gmail.com> wrote:

> These are reported by make --warn-undefined-variables.
>
> Most were being set previously (sometimes 20 years ago) and got left
> behind in recepies after their definitions have been removed. Others
> only get set in some configurations so it makes sense to prevent them
> from inheriting stray values from the environment otherwise. A few are
> just typos.
>

I've been looking at the Makefiles as well, and I find I have a lot of
questions, like:

   1. What is the point of ‘$(BUILD_DIR)’?
   When is it not simply ‘$$PWD’ or ‘.’ (prefaced by one or more ‘../’ when
   in a subdirectory)?
   2. Do any of the following *ever* change?
   dot = .
   DEFDIR = builtins
   LIBBUILD = lib
   GLOB_LIBDIR = lib/glob
   INTL_LIBDIR = lib/intl
   ALLOC_LIBDIR = lib/malloc
   RL_LIBDIR = lib/readline
   HIST_LIBDIR = lib/readline *(again)*
   SH_LIBDIR = lib/sh
   TERM_LIBDIR = lib/termcap
   TILDE_LIBDIR = lib/tilde
   3. Are any of these *ever* used?
   ALLOC_ABSSRC = ${topdir}/$(ALLOC_LIBDIR)
   BASE_CFLAGS_FOR_BUILD = @BASE_CFLAGS_FOR_BUILD@
   BUILD_INCLUDED_LIBINTL = @BUILD_INCLUDED_LIBINTL@
   BUILTIN_ABSSRC = ${topdir}/builtins
   DEBUGGER_DIR = debugger
   DOCSRC = $(srcdir)/doc
   EXTRASOURCES = *(a long list)*
   GCC_LINT_CFLAGS = $(BASE_CCFLAGS) $(CPPFLAGS) $(GCC_LINT_FLAGS)
   GLOB_ABSSRC = ${topdir}/$(GLOB_LIBDIR)
   GLOB_OBJ = *(a long list)*
   HISTORY_OBJ = *(a long list)*
   HIST_ABSSRC = ${topdir}/$(HIST_LIBDIR)
   HIST_INCLUDEDIR = @HIST_INCLUDEDIR@
   INTLLIBS = @INTLLIBS@
   INTLOBJS = @INTLOBJS@
   INTL_ABSSRC = ${topdir}/$(INTL_LIB)
   LIBINTL = @LIBINTL@
   LTLIBINTL = @LTLIBINTL@
   MALLOC = @MALLOC@
   OTHERS = *(a list)*
   PACKAGE_BUGREPORT = @PACKAGE_BUGREPORT@
   PACKAGE_NAME = @PACKAGE_NAME@
   PACKAGE_STRING = @PACKAGE_STRING@
   PACKAGE_TARNAME = @PACKAGE_TARNAME@
   PO_SRC = $(srcdir)/po/
   PSIZE_SOURCE    = *(a long list)*
   READLINE_OBJ    = *(a long list)*
   RELOCATABLE_DEFS = -DENABLE_RELOCATABLE=1 -DIN_LIBRARY \
   RL_ABSSRC = ${topdir}/$(RL_LIBDIR)
   RL_INCLUDEDIR = @RL_INCLUDEDIR@
   RL_LIBDOC = $(RL_LIBSRC)/doc
   SH_ABSSRC = ${topdir}/${SH_LIBSRC}
   SIGNAMES_SUPPORT = $(SUPPORT_SRC)mksignames.c
   SRC = *(a short list)*
   SRC1 =  man2html.c
   STATIC_SOURCE   = *(a long list)*
   STUB_SOURCE = stub.c
   SUBDIR_INCLUDES = -I. @RL_INCLUDE@ -I$(topdir) -I$(topdir)/$(LIBSUBDIR)
   TERMCAP_LDFLAGS = -L$(TERM_LIBDIR)
   TERMCAP_OBJ     = $(TERM_LIBDIR)/termcap.o \
   TERM_ABSSRC = ${topdir}/$(TERM_LIBDIR)
   TEX         = tex
   TEXINDEX    = texindex
   TILDE_ABSSRC = ${topdir}/$(TILDE_LIBDIR)
   TILDE_OBJ       = $(TILDE_LIBDIR)/tilde.o
   USE_INCLUDED_LIBINTL = @USE_INCLUDED_LIBINTL@
   datarootdir = @datarootdir@
   host_cpu = @host_cpu@
   host_vendor = @host_vendor@
   l = @INTL_LIBTOOL_SUFFIX_PREFIX@
   transform = @program_transform_name@


I think I may have identified some issues as well.

Firstly, almost all targets are simply given as a path relative to the
current directory (which is fine) but where they're noted as dependencies,
they're often prefaced by some ‘$(FOOLIB)/’.

I'm guessing that the idea was that you could link with a different version
of the library outside the build tree (and outside the source tree), but is
that actually used anywhere?
If it *is* used, I'd ideally like to see all the variables that reference
build directories have trailing slashes, unless they're completely empty
(meaning the current directory). This would probably need some adjustment
to the ‘configure’ script, or to autoconf.

Otherwise I'd rather just hard-code them as relative paths; I have a patch
to that effect if you're interested.

I'm seeing weird errors where generated dependencies wind up including both
'./foo.h' and 'foo.h' separately. Having the wrong one is a problem when
foo.h is actually a generated file, in which case the addition or removal
of './' can thwart the rule intended to build it, or even cause ‘make’ to
abort due to the absence of a stated prerequisite that has no rule.
(Depending on the version of Make, it may or may not normalize file paths.)

Secondly, where targets are built by running a command in another
directory, that secondary build process can inherit paths that no longer
point to the correct place.

If we could be sure that the variables are always relative, then it would
be simple enough to preface them with ‘../’, like this:

   - cd $(@D) && $(MAKE) BUILD_DIR=../$(BUILD_DIR) $(MAKEFLAGS) $(@F)


However if they could be absolute paths, that would clearly be thwarted.

The tricky part is that this may or may not affect variables that point to
the source, depending on how ‘configure’ was invoked:

   - when run in the source tree as ‘./configure’ it sets ‘BUILD_DIR=.’
   (which I would like to see changed to ‘./’ or empty);
   - when run as ‘../bash-source/configure’ it sets
   ‘BUILD_DIR=../bash-source’
   - when run as ‘/home/me/src/bash/configure’ it sets
   ‘BUILD_DIR=/home/me/src/bash’


It might be tempting to thing "always use absolute paths", but that causes
its own problems:

   - it won't work in a sandpit where the current user does not have
   permission to reach $PWD from ‘/’;
   - some of the paths wind up being embedded in the resulting executable,
   and they may contain sensitive information such as a personal username or
   home directory;

So I would like to take the opposite approach: make *maximal* use of
relative paths, and in addition, set either

   - ‘UP=../’ and ‘UPSRC=../’ when running ‘./configure’ or
   ‘../src/configure’; or
   - ‘UP=../’ and ‘UPSRC=’ (empty) when running ‘/path/to/configure’; or
   - ‘UP=’ and ‘UPSRC=’ (both empty) when doing something weird with
   BUILD_DIR

so that

   - cd $(@D) && $(MAKE) BUILD_DIR=$(UP)$(BUILD_DIR)
   top_srcdir=$(UPSRC)$(top_srcdir) $(MAKEFLAGS) $(@F)

will correctly set BUILD_DIR and TOPDIR


-Martin