diff mbox

[XTF] Correct the usage of $(DESTDIR) and $(prefix)

Message ID 1469038906-27330-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper July 20, 2016, 6:21 p.m. UTC
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(-)

Comments

Douglas Goldstein July 20, 2016, 6:28 p.m. UTC | #1
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.
Wei Liu July 20, 2016, 6:31 p.m. UTC | #2
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.
Ian Jackson July 21, 2016, 10:43 a.m. UTC | #3
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.
Andrew Cooper July 21, 2016, 10:57 a.m. UTC | #4
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
Ian Jackson July 21, 2016, 11:10 a.m. UTC | #5
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 mbox

Patch

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