mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 44945: Add autoconf tests for XFS project quotas.
Date Sat, 26 Mar 2016 06:43:20 GMT

-----------------------------------------------------------
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
> 
>


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