mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Budnik <abud...@mesosphere.com>
Subject Re: Review Request 62661: Added --disable-libtool-wrapper configuration to Mesos.
Date Thu, 28 Sep 2017 18:24:11 GMT


> On Sept. 28, 2017, 5:22 p.m., Benjamin Bannier wrote:
> > configure.ac
> > Lines 239 (patched)
> > <https://reviews.apache.org/r/62661/diff/1/?file=1838943#file1838943line239>
> >
> >     Let's call this `libtool-wrappers` to match what we use in the code (it also
refers to multiple wrappers in the end).

Fixed.


> On Sept. 28, 2017, 5:22 p.m., Benjamin Bannier wrote:
> > configure.ac
> > Lines 241-242 (patched)
> > <https://reviews.apache.org/r/62661/diff/1/?file=1838943#file1838943line241>
> >
> >     This helpstring needs to clearly call out that the resulting binaries *cannot
be installed* properly (due to the hardcoded build lib dir in `RPATH`). This is more important
that what exact flag we pass down to `libtool`, e.g., something along the lines of
> >     
> >         Directly create binaries instead of libtool wrapper scripts to prevent relinking
when first executing binaries from the build directory. This flag leads to binaries with the
build directory encoded in the executables' RPATH which makes them in general unfit for installation.
USE WITH CARE.

Ok. Fixed.


> On Sept. 28, 2017, 5:22 p.m., Benjamin Bannier wrote:
> > configure.ac
> > Lines 667 (patched)
> > <https://reviews.apache.org/r/62661/diff/1/?file=1838943#file1838943line667>
> >
> >     The control flow in the makefile might be easier to follow if we define `DISABLE_LIBTOOL_WRAPPERS`
here instead,
> >     
> >         AM_CONDITIONAL([DISABLE_LIBTOOL_WRAPPERS], [test x"enable_libtool_wrappers"
!= "xno"])

Fixed.


> On Sept. 28, 2017, 5:22 p.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 242-247 (patched)
> > <https://reviews.apache.org/r/62661/diff/1/?file=1838944#file1838944line242>
> >
> >     I'd suggest to not define this variable, but instead modify `AM_LDFLAGS`,
> >     
> >         AM_LDFLAGS += -no-install
> >     
> >     With that should we add another executable it would automatically pick up the
flag without having to set its `LDFLAGS`.

Fixed.


> On Sept. 28, 2017, 5:22 p.m., Benjamin Bannier wrote:
> > src/Makefile.am
> > Lines 1587 (patched)
> > <https://reviews.apache.org/r/62661/diff/1/?file=1838944#file1838944line1587>
> >
> >     This isn't needed if we modify the global `AM_LDFLAGS`.
> >     
> >     Similar for all instances below.

Fixed.


- Andrei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62661/#review186569
-----------------------------------------------------------


On Sept. 28, 2017, 11:37 a.m., Andrei Budnik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62661/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2017, 11:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Kapil Arya.
> 
> 
> Bugs: MESOS-7500
>     https://issues.apache.org/jira/browse/MESOS-7500
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This flag is used to force libtool to generate executables instead of
> wrapper scripts. A wrapper script might trigger relinking, which takes
> quite a while on slow machines, thus causing failure of tests.
> 
> 
> Diffs
> -----
> 
>   configure.ac 92bc1aa5f9604e3b2b678225a57622cd2eb8679a 
>   src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900 
> 
> 
> Diff: https://reviews.apache.org/r/62661/diff/2/
> 
> 
> Testing
> -------
> 
> 1. sudo make check (fedora)
> 2. Apache CI (centos 7, ubuntu 14.04)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message