On Sun, Mar 1, 2020 at 3:43 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > [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@xxxxxxxxx> wrote: > > > > On Sun, Mar 1, 2020 at 2:40 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > > > > > On Sun, Mar 1, 2020 at 9:33 PM William Roberts <bill.c.roberts@xxxxxxxxx> wrote: > > > > > > > > On Sun, Mar 1, 2020 at 2:21 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > > > > > > > > > On Sun, Mar 1, 2020 at 8:53 PM William Roberts <bill.c.roberts@xxxxxxxxx> wrote: > > > > > > > > > > > > On Sun, Mar 1, 2020 at 1:25 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > > > > > > > > > > > > > On Fri, Feb 28, 2020 at 2:32 PM <bill.c.roberts@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > From: William Roberts <william.c.roberts@xxxxxxxxx> > > > > > > > > > > > > > > > > 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.
