> On March 22, 2014, 8:14 a.m., Adam B wrote: > > configure.ac, line 250 > > <https://reviews.apache.org/r/18957/diff/2/?file=522715#file522715line250> > > > > +1 on making this easier to read. Just decoded it and here's my > > pseudocode interpretation: > > if (enable-proper) { > > if (with-foo) { add path to flags; } > > if (canFind(foo, flags-path)) { > > disable building the bundled lib; > > set -lfoo to use the preinstalled lib; > > } else { Error; } > > // Ditto for with-bar, etc. > > } else { > > use only bundled libs; > > } > > Two problems with this: > > 1. Right now we always error&abort if --enable-proper and the external > > leveldb cannot be found. We should fall back to bundled if > > --with-leveldb=path is not set. > > 2. Also, it seems weird that the --with-leveldb=path parameter gets > > ignored unless I also set --enable-proper. I propose that --enable-proper > > (or whatever we call it) only applies to auto-detecting preinstalled > > libraries; explicit --with-foo=path parameters are always used, and we > > won't fall back to auto-detecting or bundled libraries if it can't be found > > at the provided path. > > Here's what I suggest: > > if (with-foo) { add path to flags; } > > if (with-foo || enable-system-dependencies) { > > if (canFind(foo, flags-path)) { > > disable building the bundled lib; set -lfoo; > > } else if (with-foo) { > > Error(Could not find foo in the given path, aborting); > > } else { // enable-system-dependencies > > Warn(Could not find foo in system paths, using bundled); > > use bundled lib; > > } > > } else { > > use bundled lib; > > } > > // Ditto for with-bar, etc. > > Till Toenshoff wrote: > I really like this approach and it seems as if it does fit all demands. > Awesome comment Adam, thanks!
So I like most of the comments, but IMHO I'm against any auto-fallback mechanism. From a downstream packaging perspective this is forbidden. So if the user explicitly set --disable-bundled/--enable-proper/etc it *should fail*. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18957/#review38247 ----------------------------------------------------------- On March 23, 2014, 11:18 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18957/ > ----------------------------------------------------------- > > (Updated March 23, 2014, 11:18 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1071 > https://issues.apache.org/jira/browse/MESOS-1071 > > > Repository: mesos-git > > > Description > ------- > > Allows preventing the use of bundled libraries in favor of preinstalled > versions, where possible. > > --disable-bundled generally switches from bundled to preinstalled libraries. > For this specific flag, the configuration phase will attempt to locate a > preinstalled dependency. If that was not found, we show a warning and fall > back to using the bundled version. > If --disable-bundled is supplied or --with-leveldb=DIR was given but the > configuration phase could not locate the dependency, it stops with an error. > If --disable-bundled is not supplied and no --with-leveldb=DIR was given, the > bundled library is built and used. > > NOTE: This review only implements this preinstalled library usage on LevelDB. > For using the preinstalled LevelDB from a location that is not part of the > standard include and linker paths, you may specify > --with-leveldb=PREFIX_PATH. Subsequent review requests for other bundled > libraries will follow as soon as this implementation has been accepted. > > > Diffs > ----- > > 3rdparty/Makefile.am 23ff1fa > configure.ac 5404dc2 > src/Makefile.am 0775a0d > src/python/setup.py.in 02f00ef > > Diff: https://reviews.apache.org/r/18957/diff/ > > > Testing > ------- > > ../configure > make check > > installed leveldb in custom location > ../configure --with-leveldb=/CUSTOM_LOCATION > make check > > installed leveldb in common location > ../configure --disable-bundled > make check > > > Thanks, > > Till Toenshoff > >
