On Fri, Jul 27, 2012 at 6:47 PM, Paul Moore <pmo...@redhat.com> wrote:
> On Friday, July 27, 2012 04:38:53 PM tmpsan...@gmail.com wrote:
> > From: "Thiago Marcos P. Santos" <thiago.san...@intel.com>
> >
> > libseccomp should not assume the existence of the prefix and library
> > directory since it can be an empty directory to be user for testing.
> > The best we can do is check if the destination path is a file.
> >
> > We should also create the pkgconfig directory other "install" will
> > install as file name pkgconfig and not inside the folder.
> >
> > Signed-off-by: Thiago Marcos P. Santos <thiago.san...@intel.com>
>
> Hello,
>
> Thanks for the patch, I have a few comments which I've included below. Do
> you
> think you will be able to update the patch with the comments incorporated
> and
> resend?
>
> > ---
> > Makefile | 1 +
> > configure | 4 ++--
> > macros.mk | 2 +-
> > 3 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 8c414d4..b1292a0 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -85,6 +85,7 @@ tools: src
> >
> > install: $(SUBDIRS_BUILD)
> > @$(ECHO) "INFO: installing in $(INSTALL_PREFIX) ..."
> > + @$(MKDIR) $(INSTALL_LIB_DIR)/pkgconfig
> > $(INSTALL_MACRO) libseccomp.pc $(INSTALL_LIB_DIR)/pkgconfig
> > @for dir in $(SUBDIRS_INSTALL); do \
> > $(ECHO) "INFO: installing from $$dir/"; \
>
> I understand why we need this, but the problem is that the mkdir will
> create
> the directory with the current user's permissions, ignoring
> $(INSTALL_OWNER)
> and $(INSTALL_GROUP).
>
> I think the best way to solve this right now is to create a
> INSTALL_PC_MACRO
> similar to the INSTALL_LIB_MACRO but configured to use the
> $(INSTALL_LIB_DIR)/pkgconfig directory.
>
> > diff --git a/configure b/configure
> > index 8ac0b75..c3c3e23 100755
> > --- a/configure
> > +++ b/configure
> > @@ -188,14 +188,14 @@ while [[ $# -gt 0 ]]; do
> > done
> >
> > # validate the options
> > -if [[ ! -d $opt_prefix ]]; then
> > +if [[ -f $opt_prefix ]]; then
> > msg_error "install prefix ($opt_prefix) is not a directory"
> > exit 1
> > fi
> > if [[ -z $opt_libdir ]]; then
> > opt_libdir="$opt_prefix/lib"
> > fi
> > -if [[ ! -d $opt_libdir ]]; then
> > +if [[ -f $opt_libdir ]]; then
> > msg_error "libdir ($opt_libdir) is not a directory"
> > exit 1
> > fi
>
> Perhaps the right logic is to fail if the path exist and it isn't a
> directory?
>
> if [[ -e "$dir" && ! -d "$dir" ]]; then
> ...
> fi
>
> > diff --git a/macros.mk b/macros.mk
> > index 5998010..5ecccca 100644
> > --- a/macros.mk
> > +++ b/macros.mk
> > @@ -58,7 +58,7 @@ MV ?= mv
> > CAT ?= cat
> > ECHO ?= echo
> > TAR ?= tar
> > -MKDIR ?= mkdir
> > +MKDIR ?= mkdir -p
>
> Seems like a reasonable change but it should be needed it we add the new
> macro
> as mentioned above.
Sure thing. New patch coming in a second.
Thanks for reviewing.
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
libseccomp-discuss mailing list
libseccomp-discuss@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libseccomp-discuss
