[CRIU] Subject: [PATCH 2/2] Makefile variables BINDIR, SBINDIR, MANDIR, LIBDIR shall also be externally configurable.

Wed Jun 28 02:10:18 MSK 2017

2017-06-28 0:15 GMT+03:00 Francesco Giancane <francescogiancane8 at gmail.com>:
> From a2d7f43ceb7e42ef349db2c74c39fe1b037accf4 Mon Sep 17 00:00:00 2001
> From: Francesco Giancane <francescogiancane8 at gmail.com>
> Date: Tue, 27 Jun 2017 23:05:57 +0200
> Subject: [PATCH] Make the Makefile variables externally configurable.
>
> As of manual page INSTALL.md, it is stated that those variables can be
> overridden by means of environmental variables.
>
> export BINDIR="somedir"
> export SBINDIR="somedir"
> export LIBDIR="somedir"
> export MANDIR="somedir"
> export INCLUDEDIR="somedir"
> export LIBEXECDIR="somedir"
>
> make install
>
> But those settings will not be honored, sticking to default Makefile values.
> This patch fixes the issue.
>
> Signed-off-by: Francesco Giancane <francescogiancane8 at gmail.com>

Looks good to me,
Reviewed-by: Dmitry Safonov <dsafonov at virtuozzo.com>

> ---
>  Makefile.install | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/Makefile.install b/Makefile.install
> index 979ef0d..5e61849 100644
> --- a/Makefile.install
> +++ b/Makefile.install
> @@ -1,27 +1,30 @@
>  #
>  # Installation paths.
>  PREFIX := /usr/local
> -BINDIR := $(PREFIX)/bin
> -SBINDIR := $(PREFIX)/sbin
> -MANDIR := $(PREFIX)/share/man
> -LIBDIR := $(PREFIX)/lib
> -INCLUDEDIR := $(PREFIX)/include
> -LIBEXECDIR := $(PREFIX)/libexec
> +BINDIR ?= $(PREFIX)/bin
> +SBINDIR ?= $(PREFIX)/sbin
> +MANDIR ?= $(PREFIX)/share/man
> +INCLUDEDIR ?= $(PREFIX)/include
> +LIBEXECDIR ?= $(PREFIX)/libexec
>  RUNDIR ?= /run
>
>  #
>  # For recent Debian/Ubuntu with multiarch support.
>  DEB_HOST_MULTIARCH := $(shell dpkg-architecture -qDEB_HOST_MULTIARCH
> 2>/dev/null)
>  ifneq "$(DEB_HOST_MULTIARCH)" ""
> -        LIBDIR := $(PREFIX)/lib/$(DEB_HOST_MULTIARCH)
> +        LIBDIR ?= $(PREFIX)/lib/$(DEB_HOST_MULTIARCH)
>  else
>          #
>          # For most other systems
>          ifeq "$(shell uname -m)" "x86_64"
> -                LIBDIR := $(PREFIX)/lib64
> +                LIBDIR ?= $(PREFIX)/lib64
>          endif
>  endif
>
> +#
> +# LIBDIR falls back to the standard path.
> +LIBDIR ?= $(PREFIX)/lib
> +
>  export PREFIX BINDIR SBINDIR MANDIR RUNDIR
>  export LIBDIR INCLUDEDIR LIBEXECDIR
>
> --
> 2.9.4
>
> This shall do the job. Did not change the indentation, but `git diff`
> reported that the tabulation character was there in the first place.
>
> Francesco Giancane
>
>
>
> 2017-06-27 22:38 GMT+02:00 Dmitry Safonov <0x7f454c46 at gmail.com>:
>> 2017-06-27 23:22 GMT+03:00 Francesco Giancane <francescogiancane8 at gmail.com>:
>>> Francesco Giancane
>>>
>>> Via Paolo Braccini 46, 10141 Torino (TO)
>>> +393933920466
>>> https://it.linkedin.com/in/francesco-giancane-aa7213117
>>>
>>>
>>>
>>> 2017-06-27 22:08 GMT+02:00 Dmitry Safonov <0x7f454c46 at gmail.com>:
>>>> Nit: using git with `git send-email --to criu at openvz.org` will make
>>>> sending patches much easier for you. You only need to fill
>>>> your post server settings in git config.
>>>
>>> Thank you for the suggestion. Will check it out for the next time. :)
>>>>
>>>> 2017-06-27 22:51 GMT+03:00 Francesco Giancane <francescogiancane8 at gmail.com>:
>>>>> From 551e9d2f88258f42c4f2594d7c6805c486b02f99 Mon Sep 17 00:00:00 2001
>>>>> From: Francesco Giancane <francescogiancane8 at gmail.com>
>>>>> Date: Mon, 26 Jun 2017 00:50:44 +0200
>>>>> Subject: [PATCH 2/2] Makefile variables BINDIR, SBINDIR, MANDIR, LIBDIR shall
>>>>>  also be externally configurable.
>>>>>
>>>>>  As stated in the official INSTALL.md document, BINDIR, SBINDIR, MANDIR, LIBDIR
>>>>>  variables shall be configurable using environment variables.
>>>>>
>>>>> i.e.
>>>>> export BINDIR='...'
>>>>> export SBINDIR='...'
>>>>> export MANDIR='...'
>>>>> export LIBDIR='...'
>>>>>
>>>>> make install
>>>>>
>>>>>  Before this patch, they were set at fixed values. Now it is possible to change
>>>>>  them at ease.
>>>>>
>>>>>  I preserved the possibility to add the '64' suffix or the DEB_HOST_MULTIARCH
>>>>>  variable as before, so that the LIBDIR is correctly computed.
>>>>
>>>> I would expect that setting LIBDIR from the environment wouldn't get
>>>> overwritten in
>>>> the makefile. What if we make all LIBDIR assigns conditional (?=) and
>>>> move the first
>>>> one LIBDIR ?= $(PREFIX)/lib under endif for DEB_HOST_MULTIARCH.
>>>> Also, please, don't ruin space-indention.
>>>
>>> Sorry for the indentation. My vi environment is probably not
>>> configured with the same indentation rules.
>>
>> That's Ok, just keep the same formatting as in the other part of a file.
>>
>>> To be honest, the fix I propose is the first thing that came into my
>>> mind: the suffix is just in case of multi arch systems,
>>> so I thought that this should work well. Maybe your solution is even
>>> more elegant than mine. No problem by my side
>>> if you are willing to change it the way you proposed.
>>
>> Yep, so if that works for you, could you rework your patch this way?
>> Because that seems to be expected that if you specify LIBDIR,
>> the files will be installed in the directory you've specified, not in some
>> other directory that's formed based on your LIBDIR variable.
>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Francesco Giancane <francescogiancane8 at gmail.com>
>>>>> ---
>>>>>  Makefile.install | 12 ++++++------
>>>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/Makefile.install b/Makefile.install
>>>>> index 3606e1e..bd8bbd8 100644
>>>>> --- a/Makefile.install
>>>>> +++ b/Makefile.install
>>>>> @@ -1,10 +1,10 @@
>>>>>  #
>>>>>  # Installation paths.
>>>>>  PREFIX ?= /usr/local
>>>>> -BINDIR := $(PREFIX)/bin
>>>>> -SBINDIR := $(PREFIX)/sbin
>>>>> -MANDIR := $(PREFIX)/share/man
>>>>> -LIBDIR := $(PREFIX)/lib
>>>>> +BINDIR ?= $(PREFIX)/bin
>>>>> +SBINDIR ?= $(PREFIX)/sbin
>>>>> +MANDIR ?= $(PREFIX)/share/man
>>>>> +LIBDIR ?= $(PREFIX)/lib
>>>>>  INCLUDEDIR := $(PREFIX)/include
>>>>>  LIBEXECDIR := $(PREFIX)/libexec
>>>>>  RUNDIR ?= /run
>>>>> @@ -13,12 +13,12 @@ RUNDIR ?= /run
>>>>>  # For recent Debian/Ubuntu with multiarch support.
>>>>>  DEB_HOST_MULTIARCH := $(shell dpkg-architecture -qDEB_HOST_MULTIARCH
>>>>> 2>/dev/null)
>>>>>  ifneq "$(DEB_HOST_MULTIARCH)" ""
>>>>> -        LIBDIR := $(PREFIX)/lib/$(DEB_HOST_MULTIARCH)
>>>>> +LIBDIR := $(LIBDIR)/$(DEB_HOST_MULTIARCH)
>>>>>  else
>>>>>          #
>>>>>          # For most other systems
>>>>>          ifeq "$(shell uname -m)" "x86_64"
>>>>> -                LIBDIR := $(PREFIX)/lib64
>>>>> + LIBDIR := $(LIBDIR)64
>>>>>          endif
>>>>>  endif
>>>>>
>>>>> --
>>>>> 2.9.4

-- 
             Dmitry