On 19/01/18 13:07, Marcus Folkesson wrote:
> Hi Nicolas!
>
> On Wed, Jan 17, 2018 at 11:12:56PM +0100, Nicolas Iooss wrote:
>> On Tue, Jan 16, 2018 at 9:23 PM, Marcus Folkesson
>> <marcus.folkesson@xxxxxxxxx> wrote:
>>> This patch solves the following issues:
>>> - The pkg-config files generates odd paths when using DESTDIR without PREFIX
>>> - DESTDIR is needed during compile time to compute library and header paths which it should not.
>>> - Installing with both DESTDIR and PREFIX set gives us odd paths
>>> - Make usage of DESTDIR and PREFIX more standard
>>>
>>> Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx>
>>> ---
>>> libselinux/include/Makefile | 4 ++--
>>> libselinux/man/Makefile | 7 ++++---
>>> libselinux/src/Makefile | 12 +++++-------
>>> libselinux/src/libselinux.pc.in | 2 +-
>>> libselinux/utils/Makefile | 6 ++----
>>> 5 files changed, 14 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/libselinux/include/Makefile b/libselinux/include/Makefile
>>> index 757a6c9c..3b51f5ce 100644
>>> --- a/libselinux/include/Makefile
>>> +++ b/libselinux/include/Makefile
>>> @@ -1,6 +1,6 @@
>>> # Installation directories.
>>> -PREFIX ?= $(DESTDIR)/usr
>>> -INCDIR ?= $(PREFIX)/include/selinux
>>> +PREFIX ?= /usr
>>> +INCDIR = $(DESTDIR)$(PREFIX)/include/selinux
>>>
>>> all:
>>>
>>> diff --git a/libselinux/man/Makefile b/libselinux/man/Makefile
>>> index 0643e6af..233bfaa9 100644
>>> --- a/libselinux/man/Makefile
>>> +++ b/libselinux/man/Makefile
>>> @@ -1,7 +1,8 @@
>>> # Installation directories.
>>> -MAN8DIR ?= $(DESTDIR)/usr/share/man/man8
>>> -MAN5DIR ?= $(DESTDIR)/usr/share/man/man5
>>> -MAN3DIR ?= $(DESTDIR)/usr/share/man/man3
>>> +PREFIX ?= /usr
>>> +MAN8DIR ?= $(DESTDIR)$(PREFIX)/share/man/man8
>>> +MAN5DIR ?= $(DESTDIR)$(PREFIX)/share/man/man5
>>> +MAN3DIR ?= $(DESTDIR)$(PREFIX)/share/man/man3
>>>
>>> all:
>>>
>>> diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
>>> index 18df75c8..18a58164 100644
>>> --- a/libselinux/src/Makefile
>>> +++ b/libselinux/src/Makefile
>>> @@ -8,8 +8,8 @@ RUBYPREFIX ?= $(notdir $(RUBY))
>>> PKG_CONFIG ?= pkg-config
>>>
>>> # Installation directories.
>>> -PREFIX ?= $(DESTDIR)/usr
>>> -LIBDIR ?= $(PREFIX)/lib
>>> +PREFIX ?= /usr
>>> +LIBDIR ?= $(DESTDIR)$(PREFIX)/lib
>>> SHLIBDIR ?= $(DESTDIR)/lib
>>> INCLUDEDIR ?= $(PREFIX)/include
>>> PYINC ?= $(shell $(PKG_CONFIG) --cflags $(PYPREFIX))
>>> @@ -19,8 +19,6 @@ PYCEXT ?= $(shell $(PYTHON) -c 'import imp;print([s for s,m,t in imp.get_suffixe
>>> RUBYINC ?= $(shell $(RUBY) -e 'puts "-I" + RbConfig::CONFIG["rubyarchhdrdir"] + " -I" + RbConfig::CONFIG["rubyhdrdir"]')
>>> RUBYLIBS ?= $(shell $(RUBY) -e 'puts "-L" + RbConfig::CONFIG["libdir"] + " -L" + RbConfig::CONFIG["archlibdir"] + " " + RbConfig::CONFIG["LIBRUBYARG_SHARED"]')
>>> RUBYINSTALL ?= $(DESTDIR)$(shell $(RUBY) -e 'puts RbConfig::CONFIG["vendorarchdir"]')
>>> -LIBBASE ?= $(shell basename $(LIBDIR))
>>> -LIBSEPOLA ?= $(LIBDIR)/libsepol.a
>>>
>>> VERSION = $(shell cat ../VERSION)
>>> LIBVERSION = 1
>>> @@ -148,7 +146,7 @@ $(LIBSO): $(LOBJS)
>>> ln -sf $@ $(TARGET)
>>>
>>> $(LIBPC): $(LIBPC).in ../VERSION
>>> - sed -e 's/@VERSION@/$(VERSION)/; s:@prefix@:$(PREFIX):; s:@libdir@:$(LIBBASE):; s:@includedir@:$(INCLUDEDIR):; s:@PCRE_MODULE@:$(PCRE_MODULE):' < $< > $@
>>> + sed -e 's/@VERSION@/$(VERSION)/; s:@prefix@:$(PREFIX):; s:@libdir@:$(LIBDIR):; s:@includedir@:$(INCLUDEDIR):; s:@PCRE_MODULE@:$(PCRE_MODULE):' < $< > $@
>>>
>>> selinuxswig_python_exception.i: ../include/selinux/selinux.h
>>> bash -e exception.sh > $@ || (rm -f $@ ; false)
>>> @@ -156,8 +154,8 @@ selinuxswig_python_exception.i: ../include/selinux/selinux.h
>>> $(AUDIT2WHYLOBJ): audit2why.c
>>> $(CC) $(filter-out -Werror, $(CFLAGS)) $(PYINC) -fPIC -DSHARED -c -o $@ $<
>>>
>>> -$(AUDIT2WHYSO): $(AUDIT2WHYLOBJ) $(LIBSEPOLA)
>>> - $(CC) $(CFLAGS) $(LDFLAGS) -L. -shared -o $@ $^ -lselinux $(PYLIBS)
>>> +$(AUDIT2WHYSO): $(AUDIT2WHYLOBJ)
>>> + $(CC) $(CFLAGS) $(LDFLAGS) -L. -shared -o $@ $^ -lselinux $(PYLIBS) -l:libsepol.a
>>
>> Hello,
>> This change makes audit2why.so no longer being rebuilt when libsepol's
>> code change. This is an issue when debugging issues in libsepol, which
>> is why I added $(LIBSEPOLA) to the dependencies of $(AUDIT2WHYSO) in
>> commit dcd135cc06ab ("Re-link programs after libsepol.a is updated"
>> [1]).
>> By the way, I like the change from using a "hardcoded" path to
>> libsepol.a to telling the compiler to look into directories specified
>> with option -L in LDFLAGS. This would ease the packaging a little bit,
>> as it makes defining LIBSEPOLA no longer necessary (if I understood
>> the changes correctly, I have not tested this point). Is there a way
>> to ask the compiler for the resolved location of a static library, in
>> a way which can be used to compute the value of LIBSEPOLA? ("gcc
>> -Wl,--trace ..." displays it but it is not easily usable).
>
> First of all, thank you for your review.
>
> Actually, I do not have any "good" way to address this issue.
> Is $(LIBSEPOLA) mostly used during debug/development?
>
> What do you think about this change:
>
> -----------------8<--------------------------------------------
>
> diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile
> index 18df75c8..b722a584 100644
> --- a/libselinux/src/Makefile
> +++ b/libselinux/src/Makefile
> @@ -20,7 +20,11 @@ RUBYINC ?= $(shell $(RUBY) -e 'puts "-I" + RbConfig::CONFIG["rubyarchhdrdir"] +
> RUBYLIBS ?= $(shell $(RUBY) -e 'puts "-L" + RbConfig::CONFIG["libdir"] + " -L" + RbConfig::CONFIG["archlibdir"] + " " + RbConfig::CONFIG["LIBRUBYARG_SHARED"]')
> RUBYINSTALL ?= $(DESTDIR)$(shell $(RUBY) -e 'puts RbConfig::CONFIG["vendorarchdir"]')
> LIBBASE ?= $(shell basename $(LIBDIR))
> -LIBSEPOLA ?= $(LIBDIR)/libsepol.a
> +
> +# If no specific libsepol.a is specified, fall back on LDFLAGS search path
> +ifeq ($(LIBSEPOLA),)
> + LDFLAGS += -l:libsepol.a
> +endif
>
> VERSION = $(shell cat ../VERSION)
> LIBVERSION = 1
>
> -----------------8<--------------------------------------------
>
> This will search for libsepol.a in paths specified in LDFLAGS, but keep
> the possibility to "track" a specific libsepol.a to make dependent subprojects
> recompile if libsepol.a is changed.
Adding "-l:libsepol.a" to LDFLAGS breaks compiling with "make
'LDFLAGS=-Wl,--as-needed,--no-undefined'" (which helps detecting shared
libraries linking issues), because of two issues:
* "LDFLAGS += ..." does nothing when LDFLAGS is defined on the command
line. This is why "override LDFLAGS += ..." is used in other places in
the Makefile.
* After adding "override", -l:libsepol.a appears before $(AUDIT2WHYLOBJ)
in the linker invocation, so its objects get ignored because of
--as-needed (which is why LDLIBS exists independently of LDFLAGS).
This issue can be easily reproduced by running in libselinux/src the
following command:
make clean && make 'LDFLAGS=-Wl,--as-needed,--no-undefined' pywrap
In order to address this issue, I suggest defining a new variable,
LDLIBS_LIBSEPOLA, defined by something like:
# If no specific libsepol.a is specified, fall back on LDFLAGS search path
# Otherwise, as $(LIBSEPOLA) already appears in the dependencies, there
is no need to define a value for LDLIBS_LIBSEPOLA
ifeq ($(LIBSEPOLA),)
LDLIBS_LIBSEPOLA := -l:libsepol.a
endif
#....
$(AUDIT2WHYSO): $(AUDIT2WHYLOBJ) $(LIBSEPOLA)
$(CC) $(CFLAGS) $(LDFLAGS) -L. -shared -o $@ $^ -lselinux
$(LDLIBS_LIBSEPOLA) $(PYLIBS)
What do you think of this?
Best regards,
Nicolas
Attachment:
signature.asc
Description: OpenPGP digital signature
