libsemanage: check libsepol version

Commit Message

William Roberts Feb. 28, 2020, 1:32 p.m. UTC

From: William Roberts <[e-mail]>

If libsepol is older than version 3.0, the required routine
sepol_policydb_optimize will not be present. Use pkg-config to get the
modversion and check that it is 3.0 or higher.

This can manifest as a compilation issue:
direct_api.c: In function ‘semanage_direct_commit’:
direct_api.c:1466:13: error: implicit declaration of function ‘sepol_policydb_optimize’; did you mean ‘sepol_policydb_to_image’? [-Werror=implicit-function-declaration]
     retval = sepol_policydb_optimize(out);

Which is not really clear on how to check.

Signed-off-by: William Roberts <[e-mail]>
---
 libsemanage/src/Makefile | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Nicolas Iooss March 1, 2020, 7:24 p.m. UTC | #1

On Fri, Feb 28, 2020 at 2:32 PM <[e-mail]> wrote:
>
> From: William Roberts <[e-mail]>
>
> If libsepol is older than version 3.0, the required routine
> sepol_policydb_optimize will not be present. Use pkg-config to get the
> modversion and check that it is 3.0 or higher.
>
> This can manifest as a compilation issue:
> direct_api.c: In function ‘semanage_direct_commit’:
> direct_api.c:1466:13: error: implicit declaration of function ‘sepol_policydb_optimize’; did you mean ‘sepol_policydb_to_image’? [-Werror=implicit-function-declaration]
>      retval = sepol_policydb_optimize(out);
>
> Which is not really clear on how to check.

>From my point of view, this kind of dependency management is something
that is more suited in a package management system than in a Makefile.
It makes sense to check for libsepol version if there is some kind of
optional features that gets enabled according to it (in a similar way
as a --with-... statement in build systems using autoconf) or if there
is a fall back to another compatible source code (and the Makefile
would "route" the building process to the right .c file). But these
reasons do not match what you are doing in this change.

Why do you need this patch, instead of stating in the package to use
for libsemanage that it depends on libsepol>=3.0?

Thanks,
Nicolas

> Signed-off-by: William Roberts <[e-mail]>
> ---
>  libsemanage/src/Makefile | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/libsemanage/src/Makefile b/libsemanage/src/Makefile
> index f6780dc6048e..a329797fe1cc 100644
> --- a/libsemanage/src/Makefile
> +++ b/libsemanage/src/Makefile
> @@ -65,6 +65,14 @@ SWIG = swig -Wall -python -o $(SWIGCOUT) -outdir ./
>
>  SWIGRUBY = swig -Wall -ruby -o $(SWIGRUBYCOUT) -outdir ./
>
> +sepol-version-check:
> +       @v=$$($(PKG_CONFIG) --modversion libsepol); \
> +       major=$$(echo "$$v" | cut -d"." -f 1-1); \
> +       if [ "$$major" -lt 3 ]; then \
> +               >&2 echo "libsepol is required to be version 3.0 or higher, got: $$v"; \
> +               exit 1; \
> +       fi
> +
>  all: $(LIBA) $(LIBSO) $(LIBPC)
>
>  pywrap: all $(SWIGSO)
> @@ -83,12 +91,12 @@ $(SWIGSO): $(SWIGLOBJ)
>  $(SWIGRUBYSO): $(SWIGRUBYLOBJ)
>         $(CC) $(CFLAGS) $(LDFLAGS) -L. -shared -o $@ $^ -lsemanage $(RUBYLIBS)
>
> -$(LIBA): $(OBJS)
> -       $(AR) rcs $@ $^
> +$(LIBA): sepol-version-check $(OBJS)
> +       $(AR) rcs $@ $(filter-out $<,$^)
>         $(RANLIB) $@
>
> -$(LIBSO): $(LOBJS)
> -       $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
> +$(LIBSO): sepol-version-check $(LOBJS)
> +       $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $(filter-out $<,$^) -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
>         ln -sf $@ $(TARGET)
>
>  $(LIBPC): $(LIBPC).in ../VERSION
> @@ -163,4 +171,4 @@ distclean: clean
>  indent:
>         ../../scripts/Lindent $(filter-out $(GENERATED),$(wildcard *.[ch]))
>
> -.PHONY: all clean pywrap rubywrap swigify install install-pywrap install-rubywrap distclean
> +.PHONY: all clean pywrap rubywrap swigify install install-pywrap install-rubywrap distclean sepol-version-check
> --
> 2.17.1
>

William Roberts March 1, 2020, 7:53 p.m. UTC | #2

On Sun, Mar 1, 2020 at 1:25 PM Nicolas Iooss <[e-mail]> wrote:
>
> On Fri, Feb 28, 2020 at 2:32 PM <[e-mail]> wrote:
> >
> > From: William Roberts <[e-mail]>
> >
> > If libsepol is older than version 3.0, the required routine
> > sepol_policydb_optimize will not be present. Use pkg-config to get the
> > modversion and check that it is 3.0 or higher.
> >
> > This can manifest as a compilation issue:
> > direct_api.c: In function ‘semanage_direct_commit’:
> > direct_api.c:1466:13: error: implicit declaration of function ‘sepol_policydb_optimize’; did you mean ‘sepol_policydb_to_image’? [-Werror=implicit-function-declaration]
> >      retval = sepol_policydb_optimize(out);
> >
> > Which is not really clear on how to check.
>
> >From my point of view, this kind of dependency management is something
> that is more suited in a package management system than in a Makefile.
> It makes sense to check for libsepol version if there is some kind of
> optional features that gets enabled according to it (in a similar way
> as a --with-... statement in build systems using autoconf) or if there
> is a fall back to another compatible source code (and the Makefile
> would "route" the building process to the right .c file). But these
> reasons do not match what you are doing in this change.

Since we don't use autotools, selinux makefiles have to be smarter. We
already use
PKG_CONFIG to get various flags, which would normally be handled in a
configure script, which if we had autotools, would also be a place to check
dependency versions.

>
> Why do you need this patch, instead of stating in the package to use
> for libsemanage that it depends on libsepol>=3.0?

We should document that as well, but make software smarter, not people.
If we can provide a better error message, without a huge burden of
work or maintenance
I always vote to do it. In this case, it's pretty simple to do, since
we already have the infrastructure for
PKG_CONFIG in the Makefiles.

>
> Thanks,
> Nicolas
>
> > Signed-off-by: William Roberts <[e-mail]>
> > ---
> >  libsemanage/src/Makefile | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/libsemanage/src/Makefile b/libsemanage/src/Makefile
> > index f6780dc6048e..a329797fe1cc 100644
> > --- a/libsemanage/src/Makefile
> > +++ b/libsemanage/src/Makefile
> > @@ -65,6 +65,14 @@ SWIG = swig -Wall -python -o $(SWIGCOUT) -outdir ./
> >
> >  SWIGRUBY = swig -Wall -ruby -o $(SWIGRUBYCOUT) -outdir ./
> >
> > +sepol-version-check:
> > +       @v=$$($(PKG_CONFIG) --modversion libsepol); \
> > +       major=$$(echo "$$v" | cut -d"." -f 1-1); \
> > +       if [ "$$major" -lt 3 ]; then \
> > +               >&2 echo "libsepol is required to be version 3.0 or higher, got: $$v"; \
> > +               exit 1; \
> > +       fi
> > +
> >  all: $(LIBA) $(LIBSO) $(LIBPC)
> >
> >  pywrap: all $(SWIGSO)
> > @@ -83,12 +91,12 @@ $(SWIGSO): $(SWIGLOBJ)
> >  $(SWIGRUBYSO): $(SWIGRUBYLOBJ)
> >         $(CC) $(CFLAGS) $(LDFLAGS) -L. -shared -o $@ $^ -lsemanage $(RUBYLIBS)
> >
> > -$(LIBA): $(OBJS)
> > -       $(AR) rcs $@ $^
> > +$(LIBA): sepol-version-check $(OBJS)
> > +       $(AR) rcs $@ $(filter-out $<,$^)
> >         $(RANLIB) $@
> >
> > -$(LIBSO): $(LOBJS)
> > -       $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
> > +$(LIBSO): sepol-version-check $(LOBJS)
> > +       $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $(filter-out $<,$^) -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
> >         ln -sf $@ $(TARGET)
> >
> >  $(LIBPC): $(LIBPC).in ../VERSION
> > @@ -163,4 +171,4 @@ distclean: clean
> >  indent:
> >         ../../scripts/Lindent $(filter-out $(GENERATED),$(wildcard *.[ch]))
> >
> > -.PHONY: all clean pywrap rubywrap swigify install install-pywrap install-rubywrap distclean
> > +.PHONY: all clean pywrap rubywrap swigify install install-pywrap install-rubywrap distclean sepol-version-check
> > --
> > 2.17.1
> >
>

Nicolas Iooss March 1, 2020, 8:21 p.m. UTC | #3

On Sun, Mar 1, 2020 at 8:53 PM William Roberts <[e-mail]> wrote:
>
> On Sun, Mar 1, 2020 at 1:25 PM Nicolas Iooss <[e-mail]> wrote:
> >
> > On Fri, Feb 28, 2020 at 2:32 PM <[e-mail]> wrote:
> > >
> > > From: William Roberts <[e-mail]>
> > >
> > > If libsepol is older than version 3.0, the required routine
> > > sepol_policydb_optimize will not be present. Use pkg-config to get the
> > > modversion and check that it is 3.0 or higher.
> > >
> > > This can manifest as a compilation issue:
> > > direct_api.c: In function ‘semanage_direct_commit’:
> > > direct_api.c:1466:13: error: implicit declaration of function ‘sepol_policydb_optimize’; did you mean ‘sepol_policydb_to_image’? [-Werror=implicit-function-declaration]
> > >      retval = sepol_policydb_optimize(out);
> > >
> > > Which is not really clear on how to check.
> >
> > >From my point of view, this kind of dependency management is something
> > that is more suited in a package management system than in a Makefile.
> > It makes sense to check for libsepol version if there is some kind of
> > optional features that gets enabled according to it (in a similar way
> > as a --with-... statement in build systems using autoconf) or if there
> > is a fall back to another compatible source code (and the Makefile
> > would "route" the building process to the right .c file). But these
> > reasons do not match what you are doing in this change.
>
> Since we don't use autotools, selinux makefiles have to be smarter. We
> already use
> PKG_CONFIG to get various flags, which would normally be handled in a
> configure script, which if we had autotools, would also be a place to check
> dependency versions.
>
> >
> > Why do you need this patch, instead of stating in the package to use
> > for libsemanage that it depends on libsepol>=3.0?
>
> We should document that as well, but make software smarter, not people.
> If we can provide a better error message, without a huge burden of
> work or maintenance
> I always vote to do it. In this case, it's pretty simple to do, since
> we already have the infrastructure for
> PKG_CONFIG in the Makefiles.

Adding a magic Makefile target which is later removed for a variable
using $(filter-out $<,$^) makes things a little bit more complex, but
I can agree with this.
On the other hand, since I began packaging SELinux libraries for Arch
Linux, I found several releases that needed to bump such a dependency
version. For example, if I remember correctly libsemanage 2.4 requires
libsepol>=2.4, same for 2.5, 2.6... but libsemanage 2.9 could work
with libsepol 2.8 (I usually tries building with older versions when
packaging a release for Arch Linux, and the history is available for
example on https://aur.archlinux.org/cgit/aur.git/log/?h=libsemanage).

This being said, what about adding some logic to force libsemange
version X.Y to depend on libsepol>=X.Y and libselinux>=X.Y? The
version is already available in libsemanage/VERSION and this file
could be used to implement such a check.

Nicolas

William Roberts March 1, 2020, 8:33 p.m. UTC | #4

On Sun, Mar 1, 2020 at 2:21 PM Nicolas Iooss <[e-mail]> wrote:
>
> On Sun, Mar 1, 2020 at 8:53 PM William Roberts <[e-mail]> wrote:
> >
> > On Sun, Mar 1, 2020 at 1:25 PM Nicolas Iooss <[e-mail]> wrote:
> > >
> > > On Fri, Feb 28, 2020 at 2:32 PM <[e-mail]> wrote:
> > > >
> > > > From: William Roberts <[e-mail]>
> > > >
> > > > If libsepol is older than version 3.0, the required routine
> > > > sepol_policydb_optimize will not be present. Use pkg-config to get the
> > > > modversion and check that it is 3.0 or higher.
> > > >
> > > > This can manifest as a compilation issue:
> > > > direct_api.c: In function ‘semanage_direct_commit’:
> > > > direct_api.c:1466:13: error: implicit declaration of function ‘sepol_policydb_optimize’; did you mean ‘sepol_policydb_to_image’? [-Werror=implicit-function-declaration]
> > > >      retval = sepol_policydb_optimize(out);
> > > >
> > > > Which is not really clear on how to check.
> > >
> > > >From my point of view, this kind of dependency management is something
> > > that is more suited in a package management system than in a Makefile.
> > > It makes sense to check for libsepol version if there is some kind of
> > > optional features that gets enabled according to it (in a similar way
> > > as a --with-... statement in build systems using autoconf) or if there
> > > is a fall back to another compatible source code (and the Makefile
> > > would "route" the building process to the right .c file). But these
> > > reasons do not match what you are doing in this change.
> >
> > Since we don't use autotools, selinux makefiles have to be smarter. We
> > already use
> > PKG_CONFIG to get various flags, which would normally be handled in a
> > configure script, which if we had autotools, would also be a place to check
> > dependency versions.
> >
> > >
> > > Why do you need this patch, instead of stating in the package to use
> > > for libsemanage that it depends on libsepol>=3.0?
> >
> > We should document that as well, but make software smarter, not people.
> > If we can provide a better error message, without a huge burden of
> > work or maintenance
> > I always vote to do it. In this case, it's pretty simple to do, since
> > we already have the infrastructure for
> > PKG_CONFIG in the Makefiles.
>
> Adding a magic Makefile target which is later removed for a variable
> using $(filter-out $<,$^) makes things a little bit more complex, but
> I can agree with this.

I'm not a huge a fan of that, we could just make it always run on make
invocation, but I thought it was only interesting on actual builds. We don't
want to stop a make clean for instance.

> On the other hand, since I began packaging SELinux libraries for Arch
> Linux, I found several releases that needed to bump such a dependency
> version. For example, if I remember correctly libsemanage 2.4 requires
> libsepol>=2.4, same for 2.5, 2.6... but libsemanage 2.9 could work
> with libsepol 2.8 (I usually tries building with older versions when
> packaging a release for Arch Linux, and the history is available for
> example on https://aur.archlinux.org/cgit/aur.git/log/?h=libsemanage).
>
> This being said, what about adding some logic to force libsemange
> version X.Y to depend on libsepol>=X.Y and libselinux>=X.Y? The

So you want to check both libsepol and libselinux versions? So you want
me to add a libselinux version check?

> version is already available in libsemanage/VERSION and this file
> could be used to implement such a check.

I'm not following here. That file contains the version of libsemanage,
so I am not sure really how I could use that to implement the check
as we're checking for dependent package versions. Do you want to augment
that file with dependency versions?

Package config files appear to have also a requires field than can set
the minimum
version. Not sure if pkg-config has the ability to detect and error on
this condition.

>
> Nicolas
>

Nicolas Iooss March 1, 2020, 9:43 p.m. UTC | #5

[Oops, I accidentally dropped the Cc. I am restoring it now]

On Sun, Mar 1, 2020 at 10:09 PM William Roberts
<[e-mail]> wrote:
>
> On Sun, Mar 1, 2020 at 2:40 PM Nicolas Iooss <[e-mail]> wrote:
> >
> > On Sun, Mar 1, 2020 at 9:33 PM William Roberts <[e-mail]> wrote:
> > >
> > > On Sun, Mar 1, 2020 at 2:21 PM Nicolas Iooss <[e-mail]> wrote:
> > > >
> > > > On Sun, Mar 1, 2020 at 8:53 PM William Roberts <[e-mail]> wrote:
> > > > >
> > > > > On Sun, Mar 1, 2020 at 1:25 PM Nicolas Iooss <[e-mail]> wrote:
> > > > > >
> > > > > > On Fri, Feb 28, 2020 at 2:32 PM <[e-mail]> wrote:
> > > > > > >
> > > > > > > From: William Roberts <[e-mail]>
> > > > > > >
> > > > > > > If libsepol is older than version 3.0, the required routine
> > > > > > > sepol_policydb_optimize will not be present. Use pkg-config to get the
> > > > > > > modversion and check that it is 3.0 or higher.
> > > > > > >
> > > > > > > This can manifest as a compilation issue:
> > > > > > > direct_api.c: In function ‘semanage_direct_commit’:
> > > > > > > direct_api.c:1466:13: error: implicit declaration of function ‘sepol_policydb_optimize’; did you mean ‘sepol_policydb_to_image’? [-Werror=implicit-function-declaration]
> > > > > > >      retval = sepol_policydb_optimize(out);
> > > > > > >
> > > > > > > Which is not really clear on how to check.
> > > > > >
> > > > > > >From my point of view, this kind of dependency management is something
> > > > > > that is more suited in a package management system than in a Makefile.
> > > > > > It makes sense to check for libsepol version if there is some kind of
> > > > > > optional features that gets enabled according to it (in a similar way
> > > > > > as a --with-... statement in build systems using autoconf) or if there
> > > > > > is a fall back to another compatible source code (and the Makefile
> > > > > > would "route" the building process to the right .c file). But these
> > > > > > reasons do not match what you are doing in this change.
> > > > >
> > > > > Since we don't use autotools, selinux makefiles have to be smarter. We
> > > > > already use
> > > > > PKG_CONFIG to get various flags, which would normally be handled in a
> > > > > configure script, which if we had autotools, would also be a place to check
> > > > > dependency versions.
> > > > >
> > > > > >
> > > > > > Why do you need this patch, instead of stating in the package to use
> > > > > > for libsemanage that it depends on libsepol>=3.0?
> > > > >
> > > > > We should document that as well, but make software smarter, not people.
> > > > > If we can provide a better error message, without a huge burden of
> > > > > work or maintenance
> > > > > I always vote to do it. In this case, it's pretty simple to do, since
> > > > > we already have the infrastructure for
> > > > > PKG_CONFIG in the Makefiles.
> > > >
> > > > Adding a magic Makefile target which is later removed for a variable
> > > > using $(filter-out $<,$^) makes things a little bit more complex, but
> > > > I can agree with this.
> > >
> > > I'm not a huge a fan of that, we could just make it always run on make
> > > invocation, but I thought it was only interesting on actual builds. We don't
> > > want to stop a make clean for instance.
> > >
> > > > On the other hand, since I began packaging SELinux libraries for Arch
> > > > Linux, I found several releases that needed to bump such a dependency
> > > > version. For example, if I remember correctly libsemanage 2.4 requires
> > > > libsepol>=2.4, same for 2.5, 2.6... but libsemanage 2.9 could work
> > > > with libsepol 2.8 (I usually tries building with older versions when
> > > > packaging a release for Arch Linux, and the history is available for
> > > > example on https://aur.archlinux.org/cgit/aur.git/log/?h=libsemanage).
> > > >
> > > > This being said, what about adding some logic to force libsemange
> > > > version X.Y to depend on libsepol>=X.Y and libselinux>=X.Y? The
> > >
> > > So you want to check both libsepol and libselinux versions? So you want
> > > me to add a libselinux version check?
> > >
> > > > version is already available in libsemanage/VERSION and this file
> > > > could be used to implement such a check.
> > >
> > > I'm not following here. That file contains the version of libsemanage,
> > > so I am not sure really how I could use that to implement the check
> > > as we're checking for dependent package versions. Do you want to augment
> > > that file with dependency versions?
> > >
> > > Package config files appear to have also a requires field than can set
> > > the minimum
> > > version. Not sure if pkg-config has the ability to detect and error on
> > > this condition.
> >
> > My point was that it would be likely for libsemanage 3.1 to depend on
> > libsepol 3.1, libsemanage 3.2 to depend on libsepol 3.2... There is a
> > choice to be made:
> >
> > * either we introduce specific dependency checks like your patch, and
> > these checks will need to be tested and updated when appropriate,
>
> This patch should probably change to using something like:
> pkg-config --libs "bar >= 2.7"
>
> Digging into this concept, I hacked the .pc file to make it depend on
> libsepol > 5.0
>
> diff --git a/libsemanage/src/libsemanage.pc.in
> b/libsemanage/src/libsemanage.pc.in
> index 43681ddb8652..5b25e467393a 100644
> --- a/libsemanage/src/libsemanage.pc.in
> +++ b/libsemanage/src/libsemanage.pc.in
> @@ -7,7 +7,7 @@ Name: libsemanage
>  Description: SELinux management library
>  Version: @VERSION@
>  URL: http://userspace.selinuxproject.org/
> -Requires.private: libselinux libsepol
> +Requires.private: libselinux libsepol > 5.0
>  Libs: -L${libdir} -lsemanage
>  Libs.private: -lbz2
>  Cflags: -I${includedir}
>
> and ran this command:
> pkg-config --print-requires-private libsemanage
> Package 'libsemanage' requires 'libsepol > 5.0' but version of libsepol is 2.9
> You may find new versions of libsepol at http://userspace.selinuxproject.org/
>
> So we can just put this in one spot (the pc file) and we can discuss about
> running this pkg-config command in make.
>
> > * or we write down/document a rule that has been true for some
> > releases : "libsemanage X.Y depends on libsepol>=X.Y and
> > libselinux>=X.Y (where "X.Y" is the content of libsemanage/VERSION)
> > and that trying to use an older version is likely to break things".
>
> I think we want both. Documentation is nice, but clear errors under wrong
> conditions just help make things easier. Especially now that I see how to
> get this into pkg-config better.
>
> I think making the all target, which builds the LIBSO and the LIBA
> target, first build the
> PC file, run the PC check and then build the LIBSO and LIBA would be good. Then
> folks still can run the LIBSO/LIBA targets without the pkg-config checks.

By the way, there also needs to be a way to skip this check or to
configure which pkg-config file is selected when building for example
in a continuous-integration environment. For example I got a failure
in https://circleci.com/gh/fishilico/selinux/438 ("libsepol is
required to be version 3.0 or higher, got: 2.8") because libsepol 2.8
is installed system-wide while libsepol from git is being used
(because there is a -L flag in LDFLAGS). A possible way to dead with
this would be to modify an environment variable such as
PKG_CONFIG_PATH in the root Makefile (in block "ifneq ($(DESTDIR),)").

Thanks,
Nicolas

William Roberts March 1, 2020, 10:19 p.m. UTC | #6

On Sun, Mar 1, 2020 at 3:43 PM Nicolas Iooss <[e-mail]> wrote:
>
> [Oops, I accidentally dropped the Cc. I am restoring it now]
>
> On Sun, Mar 1, 2020 at 10:09 PM William Roberts
> <[e-mail]> wrote:
> >
> > On Sun, Mar 1, 2020 at 2:40 PM Nicolas Iooss <[e-mail]> wrote:
> > >
> > > On Sun, Mar 1, 2020 at 9:33 PM William Roberts <[e-mail]> wrote:
> > > >
> > > > On Sun, Mar 1, 2020 at 2:21 PM Nicolas Iooss <[e-mail]> wrote:
> > > > >
> > > > > On Sun, Mar 1, 2020 at 8:53 PM William Roberts <[e-mail]> wrote:
> > > > > >
> > > > > > On Sun, Mar 1, 2020 at 1:25 PM Nicolas Iooss <[e-mail]> wrote:
> > > > > > >
> > > > > > > On Fri, Feb 28, 2020 at 2:32 PM <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > From: William Roberts <[e-mail]>
> > > > > > > >
> > > > > > > > If libsepol is older than version 3.0, the required routine
> > > > > > > > sepol_policydb_optimize will not be present. Use pkg-config to get the
> > > > > > > > modversion and check that it is 3.0 or higher.
> > > > > > > >
> > > > > > > > This can manifest as a compilation issue:
> > > > > > > > direct_api.c: In function ‘semanage_direct_commit’:
> > > > > > > > direct_api.c:1466:13: error: implicit declaration of function ‘sepol_policydb_optimize’; did you mean ‘sepol_policydb_to_image’? [-Werror=implicit-function-declaration]
> > > > > > > >      retval = sepol_policydb_optimize(out);
> > > > > > > >
> > > > > > > > Which is not really clear on how to check.
> > > > > > >
> > > > > > > >From my point of view, this kind of dependency management is something
> > > > > > > that is more suited in a package management system than in a Makefile.
> > > > > > > It makes sense to check for libsepol version if there is some kind of
> > > > > > > optional features that gets enabled according to it (in a similar way
> > > > > > > as a --with-... statement in build systems using autoconf) or if there
> > > > > > > is a fall back to another compatible source code (and the Makefile
> > > > > > > would "route" the building process to the right .c file). But these
> > > > > > > reasons do not match what you are doing in this change.
> > > > > >
> > > > > > Since we don't use autotools, selinux makefiles have to be smarter. We
> > > > > > already use
> > > > > > PKG_CONFIG to get various flags, which would normally be handled in a
> > > > > > configure script, which if we had autotools, would also be a place to check
> > > > > > dependency versions.
> > > > > >
> > > > > > >
> > > > > > > Why do you need this patch, instead of stating in the package to use
> > > > > > > for libsemanage that it depends on libsepol>=3.0?
> > > > > >
> > > > > > We should document that as well, but make software smarter, not people.
> > > > > > If we can provide a better error message, without a huge burden of
> > > > > > work or maintenance
> > > > > > I always vote to do it. In this case, it's pretty simple to do, since
> > > > > > we already have the infrastructure for
> > > > > > PKG_CONFIG in the Makefiles.
> > > > >
> > > > > Adding a magic Makefile target which is later removed for a variable
> > > > > using $(filter-out $<,$^) makes things a little bit more complex, but
> > > > > I can agree with this.
> > > >
> > > > I'm not a huge a fan of that, we could just make it always run on make
> > > > invocation, but I thought it was only interesting on actual builds. We don't
> > > > want to stop a make clean for instance.
> > > >
> > > > > On the other hand, since I began packaging SELinux libraries for Arch
> > > > > Linux, I found several releases that needed to bump such a dependency
> > > > > version. For example, if I remember correctly libsemanage 2.4 requires
> > > > > libsepol>=2.4, same for 2.5, 2.6... but libsemanage 2.9 could work
> > > > > with libsepol 2.8 (I usually tries building with older versions when
> > > > > packaging a release for Arch Linux, and the history is available for
> > > > > example on https://aur.archlinux.org/cgit/aur.git/log/?h=libsemanage).
> > > > >
> > > > > This being said, what about adding some logic to force libsemange
> > > > > version X.Y to depend on libsepol>=X.Y and libselinux>=X.Y? The
> > > >
> > > > So you want to check both libsepol and libselinux versions? So you want
> > > > me to add a libselinux version check?
> > > >
> > > > > version is already available in libsemanage/VERSION and this file
> > > > > could be used to implement such a check.
> > > >
> > > > I'm not following here. That file contains the version of libsemanage,
> > > > so I am not sure really how I could use that to implement the check
> > > > as we're checking for dependent package versions. Do you want to augment
> > > > that file with dependency versions?
> > > >
> > > > Package config files appear to have also a requires field than can set
> > > > the minimum
> > > > version. Not sure if pkg-config has the ability to detect and error on
> > > > this condition.
> > >
> > > My point was that it would be likely for libsemanage 3.1 to depend on
> > > libsepol 3.1, libsemanage 3.2 to depend on libsepol 3.2... There is a
> > > choice to be made:
> > >
> > > * either we introduce specific dependency checks like your patch, and
> > > these checks will need to be tested and updated when appropriate,
> >
> > This patch should probably change to using something like:
> > pkg-config --libs "bar >= 2.7"
> >
> > Digging into this concept, I hacked the .pc file to make it depend on
> > libsepol > 5.0
> >
> > diff --git a/libsemanage/src/libsemanage.pc.in
> > b/libsemanage/src/libsemanage.pc.in
> > index 43681ddb8652..5b25e467393a 100644
> > --- a/libsemanage/src/libsemanage.pc.in
> > +++ b/libsemanage/src/libsemanage.pc.in
> > @@ -7,7 +7,7 @@ Name: libsemanage
> >  Description: SELinux management library
> >  Version: @VERSION@
> >  URL: http://userspace.selinuxproject.org/
> > -Requires.private: libselinux libsepol
> > +Requires.private: libselinux libsepol > 5.0
> >  Libs: -L${libdir} -lsemanage
> >  Libs.private: -lbz2
> >  Cflags: -I${includedir}
> >
> > and ran this command:
> > pkg-config --print-requires-private libsemanage
> > Package 'libsemanage' requires 'libsepol > 5.0' but version of libsepol is 2.9
> > You may find new versions of libsepol at http://userspace.selinuxproject.org/
> >
> > So we can just put this in one spot (the pc file) and we can discuss about
> > running this pkg-config command in make.
> >
> > > * or we write down/document a rule that has been true for some
> > > releases : "libsemanage X.Y depends on libsepol>=X.Y and
> > > libselinux>=X.Y (where "X.Y" is the content of libsemanage/VERSION)
> > > and that trying to use an older version is likely to break things".
> >
> > I think we want both. Documentation is nice, but clear errors under wrong
> > conditions just help make things easier. Especially now that I see how to
> > get this into pkg-config better.
> >
> > I think making the all target, which builds the LIBSO and the LIBA
> > target, first build the
> > PC file, run the PC check and then build the LIBSO and LIBA would be good. Then
> > folks still can run the LIBSO/LIBA targets without the pkg-config checks.
>
> By the way, there also needs to be a way to skip this check or to
> configure which pkg-config file is selected when building for example
> in a continuous-integration environment. For example I got a failure
> in https://circleci.com/gh/fishilico/selinux/438 ("libsepol is
> required to be version 3.0 or higher, got: 2.8") because libsepol 2.8
> is installed system-wide while libsepol from git is being used
> (because there is a -L flag in LDFLAGS). A possible way to dead with
> this would be to modify an environment variable such as
> PKG_CONFIG_PATH in the root Makefile (in block "ifneq ($(DESTDIR),)").

The default when invoking make is to build and link against the normal
search paths,
so if your building and linking against something non-standard by
modifying the various
environment flags to manipulate things like -,  etc, the
PKG_CONFIG_PATH would be
what you would also modify. If you add to the env variable, that dir
is searched first.

I'm also noticing our PC requires field is missing libaudit, ie:
requires.private audit

It looks like something like this when building the PC file is what we need:
pkg-config --print-errors ./src/libsemanage.pc << use the local file

This would also provide validation to our PC file before release.

Stephen Smalley March 2, 2020, 3:24 p.m. UTC | #7

On Sun, Mar 1, 2020 at 4:43 PM Nicolas Iooss <[e-mail]> wrote:
> > > > > On the other hand, since I began packaging SELinux libraries for Arch
> > > > > Linux, I found several releases that needed to bump such a dependency
> > > > > version. For example, if I remember correctly libsemanage 2.4 requires
> > > > > libsepol>=2.4, same for 2.5, 2.6... but libsemanage 2.9 could work
> > > > > with libsepol 2.8 (I usually tries building with older versions when
> > > > > packaging a release for Arch Linux, and the history is available for
> > > > > example on https://aur.archlinux.org/cgit/aur.git/log/?h=libsemanage).

In practice, there is really only a single version for the entire
selinux userspace these days, and one should always
upgrade all components at the same time.

> > diff --git a/libsemanage/src/libsemanage.pc.in
> > b/libsemanage/src/libsemanage.pc.in
> > index 43681ddb8652..5b25e467393a 100644
> > --- a/libsemanage/src/libsemanage.pc.in
> > +++ b/libsemanage/src/libsemanage.pc.in
> > @@ -7,7 +7,7 @@ Name: libsemanage
> >  Description: SELinux management library
> >  Version: @VERSION@
> >  URL: http://userspace.selinuxproject.org/

Not related to this per se, but these URLs need to be updated to point
to the GitHub releases.

William Roberts March 2, 2020, 3:32 p.m. UTC | #8

On Mon, Mar 2, 2020 at 9:22 AM Stephen Smalley
<[e-mail]> wrote:
>
> On Sun, Mar 1, 2020 at 4:43 PM Nicolas Iooss <[e-mail]> wrote:
> > > > > > On the other hand, since I began packaging SELinux libraries for Arch
> > > > > > Linux, I found several releases that needed to bump such a dependency
> > > > > > version. For example, if I remember correctly libsemanage 2.4 requires
> > > > > > libsepol>=2.4, same for 2.5, 2.6... but libsemanage 2.9 could work
> > > > > > with libsepol 2.8 (I usually tries building with older versions when
> > > > > > packaging a release for Arch Linux, and the history is available for
> > > > > > example on https://aur.archlinux.org/cgit/aur.git/log/?h=libsemanage).
>
> In practice, there is really only a single version for the entire
> selinux userspace these days, and one should always
> upgrade all components at the same time.

I guess we could take the VERSION file for dependencies and use this
in the requires private dependency versions and then run the
pkg-config tool to make sure they we're building against the right
version and that the PC file has no other errors.

>
> > > diff --git a/libsemanage/src/libsemanage.pc.in
> > > b/libsemanage/src/libsemanage.pc.in
> > > index 43681ddb8652..5b25e467393a 100644
> > > --- a/libsemanage/src/libsemanage.pc.in
> > > +++ b/libsemanage/src/libsemanage.pc.in
> > > @@ -7,7 +7,7 @@ Name: libsemanage
> > >  Description: SELinux management library
> > >  Version: @VERSION@
> > >  URL: http://userspace.selinuxproject.org/
>
> Not related to this per se, but these URLs need to be updated to point
> to the GitHub releases.

Sounds like the PC files could use some general housekeeping.

Ill put some more patches together at some point this week.

Patch

diff --git a/libsemanage/src/Makefile b/libsemanage/src/Makefile
index f6780dc6048e..a329797fe1cc 100644
--- a/libsemanage/src/Makefile
+++ b/libsemanage/src/Makefile
@@ -65,6 +65,14 @@  SWIG = swig -Wall -python -o $(SWIGCOUT) -outdir ./
 
 SWIGRUBY = swig -Wall -ruby -o $(SWIGRUBYCOUT) -outdir ./
 
+sepol-version-check:
+	@v=$$($(PKG_CONFIG) --modversion libsepol); \
+	major=$$(echo "$$v" | cut -d"." -f 1-1); \
+	if [ "$$major" -lt 3 ]; then \
+		>&2 echo "libsepol is required to be version 3.0 or higher, got: $$v"; \
+		exit 1; \
+	fi
+
 all: $(LIBA) $(LIBSO) $(LIBPC)
 
 pywrap: all $(SWIGSO)
@@ -83,12 +91,12 @@  $(SWIGSO): $(SWIGLOBJ)
 $(SWIGRUBYSO): $(SWIGRUBYLOBJ)
 	$(CC) $(CFLAGS) $(LDFLAGS) -L. -shared -o $@ $^ -lsemanage $(RUBYLIBS)
 
-$(LIBA): $(OBJS)
-	$(AR) rcs $@ $^
+$(LIBA): sepol-version-check $(OBJS)
+	$(AR) rcs $@ $(filter-out $<,$^)
 	$(RANLIB) $@
 
-$(LIBSO): $(LOBJS)
-	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
+$(LIBSO): sepol-version-check $(LOBJS)
+	$(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $(filter-out $<,$^) -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs
 	ln -sf $@ $(TARGET)
 
 $(LIBPC): $(LIBPC).in ../VERSION
@@ -163,4 +171,4 @@  distclean: clean
 indent:
 	../../scripts/Lindent $(filter-out $(GENERATED),$(wildcard *.[ch]))
 
-.PHONY: all clean pywrap rubywrap swigify install install-pywrap install-rubywrap distclean
+.PHONY: all clean pywrap rubywrap swigify install install-pywrap install-rubywrap distclean sepol-version-check