Patchwork [01/17] libsepol: new package

login
register
mail settings
Submitter Clayton Shotwell
Date Sept. 4, 2013, 11:09 p.m.
Message ID <1378336196-27403-2-git-send-email-clshotwe@rockwellcollins.com>
Download mbox | patch
Permalink /patch/272745/
State Superseded
Headers show

Comments

Clayton Shotwell - Sept. 4, 2013, 11:09 p.m.
Signed-off-by: Clayton Shotwell <clshotwe@rockwellcollins.com>
---
 package/Config.in            |    4 ++
 package/libsepol/Config.in   |    7 ++++
 package/libsepol/libsepol.mk |   69 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+), 0 deletions(-)
 create mode 100644 package/libsepol/Config.in
 create mode 100644 package/libsepol/libsepol.mk
Thomas Petazzoni - Sept. 5, 2013, 7:44 a.m.
Dear Clayton Shotwell,

On Wed, 4 Sep 2013 18:09:40 -0500, Clayton Shotwell wrote:

> +menu "Security"
> +source "package/libsepol/Config.in"
> +endmenu

You're introducing this new menu directly under "Target packages",
while this package (and a few of the others you're adding) are
libraries, so they should be under "Target packages -> Libraries".

I have nothing against adding both "Target packages -> Security" and
"Target packages -> Libraries -> Security", but that would require a
quick look at the existing packages that would also fit in those new
categories.

>  menu "System tools"
>  source "package/acl/Config.in"
>  source "package/attr/Config.in"
> diff --git a/package/libsepol/Config.in b/package/libsepol/Config.in
> new file mode 100644
> index 0000000..feb7f39
> --- /dev/null
> +++ b/package/libsepol/Config.in
> @@ -0,0 +1,7 @@
> +config BR2_PACKAGE_LIBSEPOL
> +	bool "libsepol"
> +	help
> +	  Libsepol is the binary policy manipulation library. It doesn't 
> +	  depend upon or use any of the other SELinux components.
> +	  
> +	  http://selinuxproject.org/page/Main_Page
> diff --git a/package/libsepol/libsepol.mk b/package/libsepol/libsepol.mk
> new file mode 100644
> index 0000000..59ca4bb
> --- /dev/null
> +++ b/package/libsepol/libsepol.mk
> @@ -0,0 +1,69 @@
> +#############################################################
> +#
> +# libsepol
> +#
> +#############################################################

Nitpick: those ### lines should have 80 dashes, and there should be one
empty line between this header and the first variables.

> +LIBSEPOL_VERSION = 2.1.9
> +LIBSEPOL_SOURCE = libsepol-$(LIBSEPOL_VERSION).tar.gz

This last line is not needed, since it's the default.

> +LIBSEPOL_SITE = http://userspace.selinuxproject.org/releases/20130423/
> +LIBSEPOL_LICENSE = LGPLv2.1

Is it really LGPLv2.1 or LGPLv2.1+ ? It's never said in the COPYING
file, you'd have to look at the copyright headers without the source
code.

> +LIBSEPOL_LICENSE_FILES = COPYING
> +
> +##############################
> +# Target Section
> +##############################

We generally don't put such delimiters.

> +LIBSEPOL_INSTALL_STAGING = YES
> +LIBSEPOL_INSTALL_TARGET = YES

This last line is not needed since it's the default.

> +LIBSEPOL_MAKE_CMDS = $(TARGET_CONFIGURE_OPTS)

I believe this definition is useless, just use directly
$(TARGET_CONFIGURE_OPTS) where appropriate.

> +
> +define LIBSEPOL_BUILD_CMDS
> +	$(MAKE) -C $(@D) $(LIBSEPOL_MAKE_CMDS) DESTDIR=$(STAGING_DIR)

DESTDIR is most likely not needed in the build step.

> +endef
> +
> +define LIBSEPOL_INSTALL_STAGING_CMDS
> +	$(MAKE) -C $(@D) install $(LIBSEPOL_MAKE_CMDS) DESTDIR=$(STAGING_DIR)
> +endef
> +
> +define LIBSEPOL_INSTALL_TARGET_CMDS
> +	$(MAKE) -C $(@D) install $(LIBSEPOL_MAKE_CMDS) DESTDIR=$(TARGET_DIR)
> +endef
> +
> +define LIBSEPOL_CLEAN_CMDS
> +	$(MAKE) -C $(@D) clean
> +endef
> +
> +define LIBSEPOL_UNINSTALL_STAGING_CMDS
> +	rm -rf $(addprefix $(STAGING_DIR),/usr/include/sepol /usr/bin/chkcon \
> +		/usr/lib/pkgconfig/libsepol* /lib/libsepol* /usr/lib/libsepol*)
> +	rm -f $(addprefix $(STAGING_DIR)/usr/man/man3/,$(notdir $(wildcard $(@D)/man/man3/*.3)))
> +	rm -f $(addprefix $(STAGING_DIR)/usr/man/man3/,$(notdir $(wildcard $(@D)/man/man8/*.8)))
> +endef
> +
> +define LIBSEPOL_UNINSTALL_TARGET_CMDS
> +	rm -rf $(addprefix $(TARGET_DIR),/usr/bin/chkcon /usr/lib/pkgconfig/libsepol* \
> +		/lib/libsepol* /usr/lib/libsepol*)
> +endef

Don't bother implementing the uninstall commands, we are phasing them
out.

> +
> +##############################
> +# Host Section
> +##############################

Header unneeded.

> +HOST_LIBSEPOL_MAKE_CMDS = $(HOST_CONFIGURE_OPTS)

Just use $(HOST_CONFIGURE_OPTS) where needed.

> +
> +define HOST_LIBSEPOL_BUILD_CMDS
> +	$(MAKE) -C $(@D) $(HOST_LIBSEPOL_MAKE_CMDS) DESTDIR=$(HOST_DIR)

DESTDIR generally not needed in the build step.

> +endef
> +
> +define HOST_LIBSEPOL_INSTALL_CMDS
> +	$(MAKE) -C $(@D) install $(HOST_LIBSEPOL_MAKE_CMDS) DESTDIR=$(HOST_DIR)
> +	mv $(HOST_DIR)/lib/libsepol.so.1 $(HOST_DIR)/usr/lib
> +	(cd $(HOST_DIR)/usr/lib; rm -f libsepol.so; ln -s libsepol.so.1 libsepol.so)
> +	-rmdir $(HOST_DIR)/lib

So I guess the problem here is that the library gets installed in /lib
while you wanted it in /usr/lib. It's not very pretty but maybe you can
cheat by passing DESTDIR=$(HOST_DIR)/usr.

> +endef
> +
> +define HOST_LIBSEPOL_CLEAN_CMDS
> +	$(MAKE) -C $(@D) clean
> +endef
> +
> +$(eval $(generic-package))
> +$(eval $(host-generic-package))

Thanks!

Thomas
Clayton Shotwell - Sept. 5, 2013, 12:58 p.m.
Thomas,

Thanks for the quick review.  I have a few questions before I submit a new 
version of 
the patch.

Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote on 09/05/2013 
02:44:46 AM:

> From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> You're introducing this new menu directly under "Target packages",
> while this package (and a few of the others you're adding) are
> libraries, so they should be under "Target packages -> Libraries".
> 
> I have nothing against adding both "Target packages -> Security" and
> "Target packages -> Libraries -> Security", but that would require a
> quick look at the existing packages that would also fit in those new
> categories.

I was hoping to keep all of the SELinux packages in one place to make it 
easier
to enable everything but I can move the libraries into a 
"Target packages -> Libraries -> Security" instead.

> > +LIBSEPOL_SITE = 
http://userspace.selinuxproject.org/releases/20130423/
> > +LIBSEPOL_LICENSE = LGPLv2.1
> 
> Is it really LGPLv2.1 or LGPLv2.1+ ? It's never said in the COPYING
> file, you'd have to look at the copyright headers without the source
> code.

I checked the source files and it is definitely LGPLv2.1+. I'll make that 
change.

> > +define LIBSEPOL_BUILD_CMDS
> > +   $(MAKE) -C $(@D) $(LIBSEPOL_MAKE_CMDS) DESTDIR=$(STAGING_DIR)
> 
> DESTDIR is most likely not needed in the build step.

Unfortunately, the Makefiles in the SELinux packages require DESTDIR to be 
specified
to determine setup include and library paths.  I thought it would be 
easier
to just work with the existing Makefiles rather than to rewrite them.

> > +define HOST_LIBSEPOL_INSTALL_CMDS
> > +   $(MAKE) -C $(@D) install $(HOST_LIBSEPOL_MAKE_CMDS) 
DESTDIR=$(HOST_DIR)
> > +   mv $(HOST_DIR)/lib/libsepol.so.1 $(HOST_DIR)/usr/lib
> > +   (cd $(HOST_DIR)/usr/lib; rm -f libsepol.so; ln -s libsepol.so.
> 1 libsepol.so)
> > +   -rmdir $(HOST_DIR)/lib
> 
> So I guess the problem here is that the library gets installed in /lib
> while you wanted it in /usr/lib. It's not very pretty but maybe you can
> cheat by passing DESTDIR=$(HOST_DIR)/usr.

Oh I wish these packages followed standard conventions.  They are 
purposefully 
installing the library in /lib and symlinking to it from /usr/lib. This 
little 
hack was created to correct this to match what most other packages do. 
If it is alright, I will just leave it like that and not worry about it 
(i.e. have the library install to /lib), otherwise I can patch the 
makefile 
to make it a little cleaner.

Thanks,
Clayton

Clayton Shotwell
Software Engineer
clshotwe@rockwellcollins.com
www.rockwellcollins.com
Thomas Petazzoni - Sept. 5, 2013, 1:19 p.m.
Clayton,

On Thu, 5 Sep 2013 07:58:28 -0500, clshotwe@rockwellcollins.com wrote:

> Thanks for the quick review.  I have a few questions before I submit a new 
> version of the patch.

Sure. Note that many of the comments I did on the first three patches
were identical, and apply to other patches in the series. If you could
rework the other patches according to those general comments, it would
be great (even though there will probably be other details to sort out
than those general comments).

> > I have nothing against adding both "Target packages -> Security" and
> > "Target packages -> Libraries -> Security", but that would require a
> > quick look at the existing packages that would also fit in those new
> > categories.
> 
> I was hoping to keep all of the SELinux packages in one place to make it 
> easier
> to enable everything but I can move the libraries into a 
> "Target packages -> Libraries -> Security" instead.

Well, libraries are normally 'selected' by the applications needing
them. So having the libraries separated from the programs is usually
not a big problem for the user, since library dependencies are
automatically pulled in when programs are enabled.

Moreover, we might end up later with a global knob 'I want SELinux' on
my system (depending on how the SELinux integration will look like),
and we can make that enable all the relevant packages automatically.

> > Is it really LGPLv2.1 or LGPLv2.1+ ? It's never said in the COPYING
> > file, you'd have to look at the copyright headers without the source
> > code.
> 
> I checked the source files and it is definitely LGPLv2.1+. I'll make that 
> change.

Ok.

> 
> > > +define LIBSEPOL_BUILD_CMDS
> > > +   $(MAKE) -C $(@D) $(LIBSEPOL_MAKE_CMDS) DESTDIR=$(STAGING_DIR)
> > 
> > DESTDIR is most likely not needed in the build step.
> 
> Unfortunately, the Makefiles in the SELinux packages require DESTDIR to be 
> specified
> to determine setup include and library paths.  I thought it would be 
> easier
> to just work with the existing Makefiles rather than to rewrite them.

Ah, right. libsepol isn't really concerned by this since it doesn't
have any dependency, but libselinux is indeed using DESTDIR at build
time to pass some include or library paths. It's a mistake of the build
system to think that the paths where things are at build time will be
the same as the one where things will be at run time, but anyway, in
this case it doesn't to cause any problem.

So, fine for passing DESTDIR= at build time, but since it's unusual,
maybe a comment above the BUILD_CMDS would be nice, like:

# DESTDIR= is needed at build time, as it's used by the Makefile to
# compute some library and header paths

(don't hesitate to fix my broken English as needed).

> > > +define HOST_LIBSEPOL_INSTALL_CMDS
> > > +   $(MAKE) -C $(@D) install $(HOST_LIBSEPOL_MAKE_CMDS) 
> DESTDIR=$(HOST_DIR)
> > > +   mv $(HOST_DIR)/lib/libsepol.so.1 $(HOST_DIR)/usr/lib
> > > +   (cd $(HOST_DIR)/usr/lib; rm -f libsepol.so; ln -s libsepol.so.
> > 1 libsepol.so)
> > > +   -rmdir $(HOST_DIR)/lib
> > 
> > So I guess the problem here is that the library gets installed in /lib
> > while you wanted it in /usr/lib. It's not very pretty but maybe you can
> > cheat by passing DESTDIR=$(HOST_DIR)/usr.
> 
> Oh I wish these packages followed standard conventions.  They are 
> purposefully 
> installing the library in /lib and symlinking to it from /usr/lib. This 
> little 
> hack was created to correct this to match what most other packages do. 
> If it is alright, I will just leave it like that and not worry about it 
> (i.e. have the library install to /lib), otherwise I can patch the 
> makefile to make it a little cleaner.

Ok, then probably what you did looks like the best compromise. Maybe a
little comment above that explains what you're doing this would be
good. Generally speaking, whenever something 'unusual' is done, it's
good to add a comment, which will help both at review time, but also at
maintenance time.

Best regards,

Thomas
Arnout Vandecappelle - Sept. 5, 2013, 4:46 p.m.
On 09/05/13 14:58, clshotwe@rockwellcollins.com wrote:
>  > > +define HOST_LIBSEPOL_INSTALL_CMDS
>  > > +   $(MAKE) -C $(@D) install $(HOST_LIBSEPOL_MAKE_CMDS)
> DESTDIR=$(HOST_DIR)
>  > > +   mv $(HOST_DIR)/lib/libsepol.so.1 $(HOST_DIR)/usr/lib
>  > > +   (cd $(HOST_DIR)/usr/lib; rm -f libsepol.so; ln -s libsepol.so.
>  > 1 libsepol.so)
>  > > +   -rmdir $(HOST_DIR)/lib
>  >
>  > So I guess the problem here is that the library gets installed in /lib
>  > while you wanted it in /usr/lib. It's not very pretty but maybe you can
>  > cheat by passing DESTDIR=$(HOST_DIR)/usr.
>
> Oh I wish these packages followed standard conventions.  They are
> purposefully
> installing the library in /lib and symlinking to it from /usr/lib. This
> little
> hack was created to correct this to match what most other packages do.

  I don't think it's really necessary to do this move just because it 
looks nicer. I think it's better to stay with upstream. Although this 
would be the first package to install anything in $(HOST_DIR)/lib, we 
anyway already have

HOST_LDFLAGS  += -L$(HOST_DIR)/lib ...

  We don't have an rpath for it, though, so maybe that should be added.

  Regards,
  Arnout
Thomas Petazzoni - Sept. 6, 2013, 6:28 a.m.
Dear Arnout Vandecappelle,

On Thu, 05 Sep 2013 18:46:22 +0200, Arnout Vandecappelle wrote:

>   I don't think it's really necessary to do this move just because it 
> looks nicer. I think it's better to stay with upstream. Although this 
> would be the first package to install anything in $(HOST_DIR)/lib, we 
> anyway already have
> 
> HOST_LDFLAGS  += -L$(HOST_DIR)/lib ...
> 
>   We don't have an rpath for it, though, so maybe that should be added.

Yes, that's the other option. I don't have a strong feeling about this.

At some point, I was thinking of proposing to remove $(HOST_DIR)/usr
and have everything under $(HOST_DIR)/ directly. The advantage is that
the binaries of the toolchain would be directly under $(HOST_DIR)/bin,
which would mean creating a tarball of $(HOST_DIR) would create a
toolchain that is more similar to other cross-compilation toolchains
you can find around (Linaro, CodeSourcery, etc.).

Thomas
Clayton Shotwell - Sept. 9, 2013, 5:36 p.m.
Thomas,

Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote on 09/06/2013 
01:28:27 AM:
> On Thu, 05 Sep 2013 18:46:22 +0200, Arnout Vandecappelle wrote:
> 
> >   I don't think it's really necessary to do this move 
> just because it 
> > looks nicer. I think it's better to stay with upstream. 
> Although this 
> > would be the first package to install anything in $
> (HOST_DIR)/lib, we 
> > anyway already have
> > 
> > HOST_LDFLAGS  += -L$(HOST_DIR)/lib ...
> > 
> >   We don't have an rpath for it, though, so maybe that 
> should be added.
> 
> Yes, that's the other option. I don't have a strong 
> feeling about this.
> 
> At some point, I was thinking of proposing to remove $(HOST_DIR)/usr
> and have everything under $(HOST_DIR)/ directly. The 
> advantage is that
> the binaries of the toolchain would be directly under $
> (HOST_DIR)/bin,
> which would mean creating a tarball of $(HOST_DIR) would create a
> toolchain that is more similar to other cross-compilation toolchains
> you can find around (Linaro, CodeSourcery, etc.).

I will go ahead and keep the move in there the way it currently is.  If 
the $(HOST_DIR)/usr folder is removed, there will have to be more cleanup 
to this package to have it not install into the $(HOST_DIR)/usr folder 
anyway.  All-in-all, this package does not follow standard install 
practices. 

Thanks,
Clayton

Clayton Shotwell
Software Engineer
clshotwe@rockwellcollins.com
www.rockwellcollins.com

Patch

diff --git a/package/Config.in b/package/Config.in
index a94cb62..21f7271 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -896,6 +896,10 @@  endif
 source "package/xmlstarlet/Config.in"
 endmenu
 
+menu "Security"
+source "package/libsepol/Config.in"
+endmenu
+
 menu "System tools"
 source "package/acl/Config.in"
 source "package/attr/Config.in"
diff --git a/package/libsepol/Config.in b/package/libsepol/Config.in
new file mode 100644
index 0000000..feb7f39
--- /dev/null
+++ b/package/libsepol/Config.in
@@ -0,0 +1,7 @@ 
+config BR2_PACKAGE_LIBSEPOL
+	bool "libsepol"
+	help
+	  Libsepol is the binary policy manipulation library. It doesn't 
+	  depend upon or use any of the other SELinux components.
+	  
+	  http://selinuxproject.org/page/Main_Page
diff --git a/package/libsepol/libsepol.mk b/package/libsepol/libsepol.mk
new file mode 100644
index 0000000..59ca4bb
--- /dev/null
+++ b/package/libsepol/libsepol.mk
@@ -0,0 +1,69 @@ 
+#############################################################
+#
+# libsepol
+#
+#############################################################
+LIBSEPOL_VERSION = 2.1.9
+LIBSEPOL_SOURCE = libsepol-$(LIBSEPOL_VERSION).tar.gz
+LIBSEPOL_SITE = http://userspace.selinuxproject.org/releases/20130423/
+LIBSEPOL_LICENSE = LGPLv2.1
+LIBSEPOL_LICENSE_FILES = COPYING
+
+##############################
+# Target Section
+##############################
+LIBSEPOL_INSTALL_STAGING = YES
+LIBSEPOL_INSTALL_TARGET = YES
+
+LIBSEPOL_MAKE_CMDS = $(TARGET_CONFIGURE_OPTS)
+
+define LIBSEPOL_BUILD_CMDS
+	$(MAKE) -C $(@D) $(LIBSEPOL_MAKE_CMDS) DESTDIR=$(STAGING_DIR)
+endef
+
+define LIBSEPOL_INSTALL_STAGING_CMDS
+	$(MAKE) -C $(@D) install $(LIBSEPOL_MAKE_CMDS) DESTDIR=$(STAGING_DIR)
+endef
+
+define LIBSEPOL_INSTALL_TARGET_CMDS
+	$(MAKE) -C $(@D) install $(LIBSEPOL_MAKE_CMDS) DESTDIR=$(TARGET_DIR)
+endef
+
+define LIBSEPOL_CLEAN_CMDS
+	$(MAKE) -C $(@D) clean
+endef
+
+define LIBSEPOL_UNINSTALL_STAGING_CMDS
+	rm -rf $(addprefix $(STAGING_DIR),/usr/include/sepol /usr/bin/chkcon \
+		/usr/lib/pkgconfig/libsepol* /lib/libsepol* /usr/lib/libsepol*)
+	rm -f $(addprefix $(STAGING_DIR)/usr/man/man3/,$(notdir $(wildcard $(@D)/man/man3/*.3)))
+	rm -f $(addprefix $(STAGING_DIR)/usr/man/man3/,$(notdir $(wildcard $(@D)/man/man8/*.8)))
+endef
+
+define LIBSEPOL_UNINSTALL_TARGET_CMDS
+	rm -rf $(addprefix $(TARGET_DIR),/usr/bin/chkcon /usr/lib/pkgconfig/libsepol* \
+		/lib/libsepol* /usr/lib/libsepol*)
+endef
+
+##############################
+# Host Section
+##############################
+HOST_LIBSEPOL_MAKE_CMDS = $(HOST_CONFIGURE_OPTS)
+
+define HOST_LIBSEPOL_BUILD_CMDS
+	$(MAKE) -C $(@D) $(HOST_LIBSEPOL_MAKE_CMDS) DESTDIR=$(HOST_DIR)
+endef
+
+define HOST_LIBSEPOL_INSTALL_CMDS
+	$(MAKE) -C $(@D) install $(HOST_LIBSEPOL_MAKE_CMDS) DESTDIR=$(HOST_DIR)
+	mv $(HOST_DIR)/lib/libsepol.so.1 $(HOST_DIR)/usr/lib
+	(cd $(HOST_DIR)/usr/lib; rm -f libsepol.so; ln -s libsepol.so.1 libsepol.so)
+	-rmdir $(HOST_DIR)/lib
+endef
+
+define HOST_LIBSEPOL_CLEAN_CMDS
+	$(MAKE) -C $(@D) clean
+endef
+
+$(eval $(generic-package))
+$(eval $(host-generic-package))