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 Wed, 30 Mar 2016 18:28:53 GMT


> On March 25, 2016, 11:43 p.m., Jiang Yan Xu wrote:
> > configure.ac, lines 957-960
> > <https://reviews.apache.org/r/44945/diff/8/?file=1311200#file1311200line957>
> >
> >     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])`
> 
> James Peach wrote:
>     No, this message should be outside the check so that the builder always knows what
happened, independent of what configure flags they passed. Typically, people start by not
passing any flags, and it is helpful to explicitly log that a feature was not enabled. Even
if you pass the --enable flags, it is comforting to get an explicit confirmation that the
feature you asked for is enabled.
> 
> Jiang Yan Xu wrote:
>     I agree that explicitly confirming that the user has enabled something is better.

>     
>     I still don't think we should print it after we've done all the dependency checks
for it and have potentially aborted configure with error messages but without telling the
error is due to "whether to enable the XFS disk isolator..."
>     
>     i.e., the following is better.
>     
>     ```
>     checking whether to enable the XFS disk isolator... yes
>     
>     OR 
>     
>     checking whether to enable the XFS disk isolator... missing libblkid headers
>     -------------------------------------------------------------------
>     Please install the libblkid development package.
>     -------------------------------------------------------------------
>     
>     OR
>     
>     checking whether to enable the XFS disk isolator... no / checking whether to enable
the XFS disk isolator... not enabled by user
>     
>     ```
>     
>     Agreed?
> 
> James Peach wrote:
>     If you do it this way, you get the headers, etc check output intermingled in the
middle of your nice clean feature check message.

I see. Seems like technology is really lacking in this! In the absence of a perfect solution,
IMO this is better:

checking whether we are asked to enable the XFS disk isolator... yes
checking for xfs/xfs.h... yes
// Move on to other headers

OR

checking whether we are asked to enable the XFS disk isolator... yes
checking for xfs/xfs.h... missing XFS quota headers
-------------------------------------------------------------------
Please install the Linux kernel headers and xfsprogs development
packages for XFS disk isolator support.
-------------------------------------------------------------------

OR 
checking whether we are asked to enable the XFS disk isolator... no
// Move on to other feature checking


TBH I haven't seen this style of using AC_MSG_CHECKING to check a user supplied flag so I
am not convinced it's idiomatic but I do agree with the comforting factor. So let's at least
make the message honest about what it's doing.

The above style still doesn't print anything when it has finished checking all of the dependencies
for XFS but it's not like autoconf has ever done a good job of this: this is lack of information.

At least we are not printing all these header checking messages and potentially fail before
we print "checking whether to enable the XFS disk isolator", which implies could be misleading.


- Jiang Yan


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


On March 29, 2016, 1:55 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> -----------------------------------------------------------
> 
> (Updated March 29, 2016, 1:55 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