| Message ID | 1469038906-27330-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7/20/16 1:21 PM, Andrew Cooper wrote: > The GNU coding standards expect $(DESTDIR) to be the root of everything > installed, and for prefix to then be added to the path. This is not how XTF > previously behaved. > > Replace $(PREFIX) with its more common form $(prefix), and rearange $(DESTDIR) > and $(prefix) to match expectation. > > Reported-by:Wei Liu <wei.liu2@citrix.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Doug Goldstein <cardoe@cardoe.com> The conclusion in the commit message is correct. The supplied patch conforms to that.
On Wed, Jul 20, 2016 at 07:21:46PM +0100, Andrew Cooper wrote: > The GNU coding standards expect $(DESTDIR) to be the root of everything > installed, and for prefix to then be added to the path. This is not how XTF > previously behaved. > > Replace $(PREFIX) with its more common form $(prefix), and rearange $(DESTDIR) "rearrange" > and $(prefix) to match expectation. > > Reported-by:Wei Liu <wei.liu2@citrix.com> Space after colon. The changes look fine to me. Wei.
Andrew Cooper writes ("[PATCH XTF] Correct the usage of $(DESTDIR) and $(prefix)"):
> The GNU coding standards expect $(DESTDIR) to be the root of everything
> installed, and for prefix to then be added to the path. This is not how XTF
> previously behaved.
>
> Replace $(PREFIX) with its more common form $(prefix), and rearange $(DESTDIR)
> and $(prefix) to match expectation.
...
> -DESTDIR ?= $(ROOT)/dist
> -PREFIX ?= $(ROOT)
> -export DESTDIR PREFIX
> +# $(prefix) defaults to $(ROOT) so development and testing can be done
> +# straight out of the working tree
> +prefix ?= $(ROOT)
...
> +DEST := $(DESTDIR)$(prefix)
...
> - @mkdir -p $(DESTDIR)
> - $(INSTALL_PROGRAM) -p xtf-runner $(DESTDIR)
> + @mkdir -p $(DEST)
> + $(INSTALL_PROGRAM) -p xtf-runner $(DEST)
The effect of this is that
make prefix=/usr
will create
/usr/xtf-runner
/usr/tests/*/test-info.json
which is not how things would normally be expected work.
I think to make this work right, you have to do something like
ifeq ($prefix,)
bindir=$(ROOT)
xtflibdir=$(ROOT)
else
bindir=$(prefix)/bin
xtflibdir=$(prefix)/lib/xtf
endif
xtftestsdir=$(xtflibdir)/tests
...
$(INSTALL_PROGRAM) -p xtf-runner $(DESTDIR)/$(bindir)
...
$(INSTALL_DATA) -p test-info.json $(DESTDIR)/$(xtftestsdir)/$(NAME)
Also it would be more conventional to use $(INSTALL) -d or
$(INSTALL_DIR) rather than mkdir.
Ian.
On 21/07/16 11:43, Ian Jackson wrote: > Andrew Cooper writes ("[PATCH XTF] Correct the usage of $(DESTDIR) and $(prefix)"): >> The GNU coding standards expect $(DESTDIR) to be the root of everything >> installed, and for prefix to then be added to the path. This is not how XTF >> previously behaved. >> >> Replace $(PREFIX) with its more common form $(prefix), and rearange $(DESTDIR) >> and $(prefix) to match expectation. > ... >> -DESTDIR ?= $(ROOT)/dist >> -PREFIX ?= $(ROOT) >> -export DESTDIR PREFIX >> +# $(prefix) defaults to $(ROOT) so development and testing can be done >> +# straight out of the working tree >> +prefix ?= $(ROOT) > ... >> +DEST := $(DESTDIR)$(prefix) > ... >> - @mkdir -p $(DESTDIR) >> - $(INSTALL_PROGRAM) -p xtf-runner $(DESTDIR) >> + @mkdir -p $(DEST) >> + $(INSTALL_PROGRAM) -p xtf-runner $(DEST) > The effect of this is that > make prefix=/usr > will create > /usr/xtf-runner > /usr/tests/*/test-info.json > which is not how things would normally be expected work. XTF doesn't match any standard installable package. It is some configuration files and a load of microkernels which are not system executables. I build it with prefix=/opt/xtf as that is the only plausible place for the results to live, per the FHS. It could certainly be argued that it should insert its own xtf/ directory rather than relying on prefix to pass it. > > I think to make this work right, you have to do something like > > ifeq ($prefix,) > bindir=$(ROOT) > xtflibdir=$(ROOT) > else > bindir=$(prefix)/bin > xtflibdir=$(prefix)/lib/xtf > endif > xtftestsdir=$(xtflibdir)/tests > ... > $(INSTALL_PROGRAM) -p xtf-runner $(DESTDIR)/$(bindir) > ... > $(INSTALL_DATA) -p test-info.json $(DESTDIR)/$(xtftestsdir)/$(NAME) xtf-runner expects to find tests/ in the same directory it resides in, because otherwise there is no sensible way to find it. > > Also it would be more conventional to use $(INSTALL) -d or > $(INSTALL_DIR) rather than mkdir. I will make this change. ~Andrew
Andrew Cooper writes ("Re: [PATCH XTF] Correct the usage of $(DESTDIR) and $(prefix)"):
> XTF doesn't match any standard installable package. It is some
> configuration files and a load of microkernels which are not system
> executables.
The question is what
really make install prefix=/usr
should do. That's something that the standard Makefile targets
suggest ought to do something sane.
From what you write it probably ought to dump its stuff in
/usr/lib/xtf/<whatever>
?
> I build it with prefix=/opt/xtf as that is the only plausible place for
> the results to live, per the FHS. It could certainly be argued that it
> should insert its own xtf/ directory rather than relying on prefix to
> pass it.
It's conventional for /opt/foo to contain /opt/foo/bin, /opt/foo/man,
and so on.
Ian.
diff --git a/Makefile b/Makefile index e21605d..c86dbb8 100644 --- a/Makefile +++ b/Makefile @@ -2,9 +2,14 @@ MAKEFLAGS += -r ROOT := $(abspath $(CURDIR)) export ROOT -DESTDIR ?= $(ROOT)/dist -PREFIX ?= $(ROOT) -export DESTDIR PREFIX +# $(prefix) defaults to $(ROOT) so development and testing can be done +# straight out of the working tree +prefix ?= $(ROOT) +DESTDIR ?= + +# Provide $(DEST) as the install root +DEST := $(DESTDIR)$(prefix) +export DEST DESTDIR prefix # Programs used CC ?= $(CROSS_COMPILE)gcc @@ -27,8 +32,8 @@ all: .PHONY: install install: - @mkdir -p $(DESTDIR) - $(INSTALL_PROGRAM) -p xtf-runner $(DESTDIR) + @mkdir -p $(DEST) + $(INSTALL_PROGRAM) -p xtf-runner $(DEST) @set -e; for D in $(wildcard tests/*); do \ [ ! -e $$D/Makefile ] && continue; \ $(MAKE) -C $$D install; \ diff --git a/build/gen.mk b/build/gen.mk index 790212b..e20fd77 100644 --- a/build/gen.mk +++ b/build/gen.mk @@ -21,6 +21,14 @@ ifneq ($(filter-out $(ALL_CATEGORIES),$(CATEGORY)),) $(error Unrecognised category '$(filter-out $(ALL_CATEGORIES),$(CATEGORY))') endif +ifeq ($(filter /%,$(prefix)),) +$(error $$(prefix) must be absolute, not '$(prefix)') +endif + +ifeq ($(filter /%,$(DEST)),) +$(error $$(DEST) must be absolute, not '$(DEST)') +endif + .PHONY: build build: $(foreach env,$(TEST-ENVS),test-$(env)-$(NAME) test-$(env)-$(NAME).cfg) build: test-info.json @@ -31,8 +39,8 @@ test-info.json: $(ROOT)/build/mkinfo.py FORCE .PHONY: install install-each-env install: install-each-env test-info.json - @mkdir -p $(DESTDIR)/tests/$(NAME) - $(INSTALL_DATA) -p test-info.json $(DESTDIR)/tests/$(NAME) + @mkdir -p $(DEST)/tests/$(NAME) + $(INSTALL_DATA) -p test-info.json $(DEST)/tests/$(NAME) define PERENV_build @@ -54,7 +62,7 @@ test-$(1)-$(NAME).cfg: $$(cfg-$(1)) FORCE @{ cat $$< $(TEST-EXTRA-CFG) ;} | \ sed -e "s/@@NAME@@/$$(NAME)/g" \ -e "s/@@ENV@@/$(1)/g" \ - -e "s!@@PREFIX@@!$$(PREFIX)!g" \ + -e "s!@@PREFIX@@!$$(prefix)!g" \ > $$@.tmp @if ! cmp -s $$@ $$@.tmp; then mv -f $$@.tmp $$@; else rm -f $$@.tmp; fi @@ -63,12 +71,12 @@ test-$(1)-$(NAME).cfg: $$(cfg-$(1)) FORCE .PHONY: install-$(1) install-$(1).cfg install-$(1): test-$(1)-$(NAME) - @mkdir -p $(DESTDIR)/tests/$(NAME) - $(INSTALL_PROGRAM) -p $$< $(DESTDIR)/tests/$(NAME) + @mkdir -p $(DEST)/tests/$(NAME) + $(INSTALL_PROGRAM) -p $$< $(DEST)/tests/$(NAME) install-$(1).cfg: test-$(1)-$(NAME).cfg - @mkdir -p $(DESTDIR)/tests/$(NAME) - $(INSTALL_DATA) -p $$< $(DESTDIR)/tests/$(NAME) + @mkdir -p $(DEST)/tests/$(NAME) + $(INSTALL_DATA) -p $$< $(DEST)/tests/$(NAME) install-each-env: install-$(1) install-$(1).cfg
The GNU coding standards expect $(DESTDIR) to be the root of everything installed, and for prefix to then be added to the path. This is not how XTF previously behaved. Replace $(PREFIX) with its more common form $(prefix), and rearange $(DESTDIR) and $(prefix) to match expectation. Reported-by:Wei Liu <wei.liu2@citrix.com> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Ian Jackson <Ian.Jackson@eu.citrix.com> CC: Wei Liu <wei.liu2@citrix.com> --- Makefile | 15 ++++++++++----- build/gen.mk | 22 +++++++++++++++------- 2 files changed, 25 insertions(+), 12 deletions(-)