----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44945/#review125457 -----------------------------------------------------------
configure.ac (line 258) <https://reviews.apache.org/r/44945/#comment188243> Two spaces to the right. Also, append "default: no" configure.ac (line 259) <https://reviews.apache.org/r/44945/#comment188248> We probably shouldn't take option arugments, i.e., `--enable-xfs-disk-isolator` is sufficient. Otherwise there is confusion with `--enable-xfs-disk-isolator=yes|true|1|absolutely`. :) s/[enable_xfs_disk_isolator=no]/[] for consistency. configure.ac (line 930) <https://reviews.apache.org/r/44945/#comment188258> Kill this blank line. Also, I know the headers below imply Linux but I guess it won't hurt to add ``` AS_IF([test "$OS_NAME" = "linux"], [], [AC_MSG_ERROR([cannot build xfs disk isolator ------------------------------------------------------------------- XFS disk isolator is only supported on Linux. ------------------------------------------------------------------- ])]) ``` so the error message is clearer. configure.ac (line 934) <https://reviews.apache.org/r/44945/#comment188262> Move one space to the right. configure.ac (line 954) <https://reviews.apache.org/r/44945/#comment188263> This could be the place we insert ``` AC_DEFINE([ENABLE_XFS_DISK_ISOLATOR]) ``` configure.ac (lines 957 - 960) <https://reviews.apache.org/r/44945/#comment188321> Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` above? i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what they don't already know (whether the system dependencies are met), not what they already know (the fact that they provided the `--enable_xfs_disk_isolator` flag). And this check is conditional: we only print `AC_MSG_CHECKING` and do these checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print `AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when all checks succeed. Would this work? If we do this, then the message could be `AC_MSG_CHECKING([whether we can enable the XFS disk isolator])` configure.ac (lines 962 - 963) <https://reviews.apache.org/r/44945/#comment188322> If we pull this up into the aforementioned place we don't need to `test "x$enable_xfs_disk_isolator" = "xyes"` repeatedly and therefore be more concise. - Jiang Yan Xu On March 22, 2016, 4:21 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44945/ > ----------------------------------------------------------- > > (Updated March 22, 2016, 4:21 p.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-4828 > https://issues.apache.org/jira/browse/MESOS-4828 > > > Repository: mesos > > > Description > ------- > > Add autoconf tests for XFS project quotas. > > > Diffs > ----- > > configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1fbbbb2d19ffb8 > > Diff: https://reviews.apache.org/r/44945/diff/ > > > Testing > ------- > > Make check. Manual verification. > > > Thanks, > > James Peach > >
