| Message ID | 1378336196-27403-2-git-send-email-clshotwe@rockwellcollins.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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
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))
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