Re: [PATCH] libsemanage: check libsepol version

[Nicolas Iooss <nicolas.iooss@m4x.org>]

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

On Sun, Mar 1, 2020 at 10:09 PM William Roberts
<bill.c.roberts@gmail.com> wrote:
>
> On Sun, Mar 1, 2020 at 2:40 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > On Sun, Mar 1, 2020 at 9:33 PM William Roberts <bill.c.roberts@gmail.com> wrote:
> > >
> > > On Sun, Mar 1, 2020 at 2:21 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> > > >
> > > > On Sun, Mar 1, 2020 at 8:53 PM William Roberts <bill.c.roberts@gmail.com> wrote:
> > > > >
> > > > > On Sun, Mar 1, 2020 at 1:25 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> > > > > >
> > > > > > On Fri, Feb 28, 2020 at 2:32 PM <bill.c.roberts@gmail.com> wrote:
> > > > > > >
> > > > > > > From: William Roberts <william.c.roberts@intel.com>
> > > > > > >
> > > > > > > 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