Re: [PATCH v2 1/4] tools lib subcmd: Add install target

[Ian Rogers]

On Mon, Nov 21, 2022 at 10:48 PM Nicolas Schier <nicolas@xxxxxxxxx> wrote:
>
> On Mon 21 Nov 2022 16:11:22 GMT, Ian Rogers wrote:
> > This allows libsubcmd to be installed as a dependency.
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/lib/subcmd/Makefile | 49 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/tools/lib/subcmd/Makefile b/tools/lib/subcmd/Makefile
> > index 8f1a09cdfd17..e96566f8991c 100644
> > --- a/tools/lib/subcmd/Makefile
> > +++ b/tools/lib/subcmd/Makefile
> > @@ -17,6 +17,15 @@ RM = rm -f
> >
> >  MAKEFLAGS += --no-print-directory
> >
> > +INSTALL = install
> > +
> > +# Use DESTDIR for installing into a different root directory.
> > +# This is useful for building a package. The program will be
> > +# installed in this directory as if it was the root directory.
> > +# Then the build tool can move it later.
> > +DESTDIR ?=
> > +DESTDIR_SQ = '$(subst ','\'',$(DESTDIR))'
> > +
> >  LIBFILE = $(OUTPUT)libsubcmd.a
> >
> >  CFLAGS := -ggdb3 -Wall -Wextra -std=gnu99 -fPIC
> > @@ -48,6 +57,18 @@ CFLAGS += $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
> >
> >  SUBCMD_IN := $(OUTPUT)libsubcmd-in.o
> >
> > +ifeq ($(LP64), 1)
> > +  libdir_relative = lib64
> > +else
> > +  libdir_relative = lib
> > +endif
> > +
> > +prefix ?=
> > +libdir = $(prefix)/$(libdir_relative)
> > +
> > +# Shell quotes
> > +libdir_SQ = $(subst ','\'',$(libdir))
> > +
> >  all:
> >
> >  export srctree OUTPUT CC LD CFLAGS V
> > @@ -61,6 +82,34 @@ $(SUBCMD_IN): FORCE
> >  $(LIBFILE): $(SUBCMD_IN)
> >       $(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(SUBCMD_IN)
> >
> > +define do_install_mkdir
> > +     if [ ! -d '$(DESTDIR_SQ)$1' ]; then             \
> > +             $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$1'; \
> > +     fi
> > +endef
> > +
> > +define do_install
> > +     if [ ! -d '$(DESTDIR_SQ)$2' ]; then             \
> > +             $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$2'; \
> > +     fi;                                             \
> > +     $(INSTALL) $1 $(if $3,-m $3,) '$(DESTDIR_SQ)$2'
> > +endef
> > +
> > +install_lib: $(LIBFILE)
> > +     $(call QUIET_INSTALL, $(LIBFILE)) \
> > +             $(call do_install_mkdir,$(libdir_SQ)); \
> > +             cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
> > +
>
> Sorry for being late.
>
> Is there a specific reason why you do not use 'mkdir -p
> $(DESTDIR)/$(2); cp $(1) $(2)' or 'install $(addprefix -m,$(3)) -D $(1)
> $(2)' for the install rules (cp.  scripts/Makefile.{dtb,mod}inst)?
>
> I think you could get rid of mkdir calls and the ..._SQ handling when
> using one of them.

Thanks for the feedback! Perhaps we can merge this as being consistent
with existing approaches:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/perf/Makefile#n185
and then refactor as you say.

> > +install_headers:
> > +     $(call QUIET_INSTALL, headers) \
>
> Unlikely, but if one of the install commands fails, you probably want
> make to exit with an error.  Might you want to add 'set -e; \' here?

Possibly. Again, I think we should aim for consistency and do this as
a follow up.

Thanks,
Ian

> Kind regards,
> Nicolas
>
>
> > +             $(call do_install,exec-cmd.h,$(prefix)/include/subcmd,644); \
> > +             $(call do_install,help.h,$(prefix)/include/subcmd,644); \
> > +             $(call do_install,pager.h,$(prefix)/include/subcmd,644); \
> > +             $(call do_install,parse-options.h,$(prefix)/include/subcmd,644); \
> > +             $(call do_install,run-command.h,$(prefix)/include/subcmd,644);
> > +
> > +install: install_lib install_headers
> > +
> >  clean:
> >       $(call QUIET_CLEAN, libsubcmd) $(RM) $(LIBFILE); \
> >       find $(or $(OUTPUT),.) -name \*.o -or -name \*.o.cmd -or -name \*.o.d | xargs $(RM)
> > --
> > 2.38.1.584.g0f3c55d4c2-goog
>
> --
> epost|xmpp: nicolas@xxxxxxxxx          irc://oftc.net/nsc
> ↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
>      -- frykten for herren er opphav til kunnskap --