You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2015/05/01 17:42:04 UTC
Review Request 33752: MESOS-2537: Fix AC_ARG_ENABLED option processing
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33752/
-----------------------------------------------------------
Review request for mesos and Cody Maloney.
Bugs: MESOS-2537
https://issues.apache.org/jira/browse/MESOS-2537
Repository: mesos
Description
-------
In a number of places, the Mesos configure script passes "$foo=yes"
to the 2nd argument of AC_ARG_ENABLED. However, the 2nd argument
is invoked when the option is provided in any form, not just when
the --enable-foo form is used. One result of this is that
--disable-optimize doesn't work.
The correct handling of the 2nd argument is to save the value of
"$enableval". This change sets the value of all the enable variables
using $enableval, and sets the default value based on the option
name.
There are a number of enable options that were internally named
"$with_foo" and "$without_foo". Rename these to "$enable_foo" for
clarity and to remove the need for both a with_ and a without_
version.
Finally, emit the compilation flags at the end of the configure
phase so it is easier to see the results of your configuration
options.
Diffs
-----
configure.ac 589ae97d0432370b462576cd1985544564893999
Diff: https://reviews.apache.org/r/33752/diff/
Testing
-------
Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify that the status summary reflects the expected compiler flags. Verify that --enable-foo and --disable-foo do different things.
Thanks,
James Peach
Re: Review Request 33752: MESOS-2537: Fix AC_ARG_ENABLED option
processing
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33752/#review82253
-----------------------------------------------------------
Patch looks great!
Reviews applied: [33752]
All tests passed.
- Mesos ReviewBot
On May 1, 2015, 3:42 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33752/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 3:42 p.m.)
>
>
> Review request for mesos and Cody Maloney.
>
>
> Bugs: MESOS-2537
> https://issues.apache.org/jira/browse/MESOS-2537
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In a number of places, the Mesos configure script passes "$foo=yes"
> to the 2nd argument of AC_ARG_ENABLED. However, the 2nd argument
> is invoked when the option is provided in any form, not just when
> the --enable-foo form is used. One result of this is that
> --disable-optimize doesn't work.
>
> The correct handling of the 2nd argument is to save the value of
> "$enableval". This change sets the value of all the enable variables
> using $enableval, and sets the default value based on the option
> name.
>
> There are a number of enable options that were internally named
> "$with_foo" and "$without_foo". Rename these to "$enable_foo" for
> clarity and to remove the need for both a with_ and a without_
> version.
>
> Finally, emit the compilation flags at the end of the configure
> phase so it is easier to see the results of your configuration
> options.
>
>
> Diffs
> -----
>
> configure.ac 589ae97d0432370b462576cd1985544564893999
>
> Diff: https://reviews.apache.org/r/33752/diff/
>
>
> Testing
> -------
>
> Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify that the status summary reflects the expected compiler flags. Verify that --enable-foo and --disable-foo do different things.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 33752: MESOS-2537: Fix AC_ARG_ENABLED option
processing
Posted by James Peach <jp...@apache.org>.
> On May 1, 2015, 7:30 p.m., Cody Maloney wrote:
> > configure.ac, line 1407
> > <https://reviews.apache.org/r/33752/diff/1/?file=947254#file947254line1407>
> >
> > I worry this changes the behavior some when --disable-bundled is set (Previously it seems like it would see if it could find the package locally, and if not enable the bundled one). Internally we don't really use that codepath though.
>
> James Peach wrote:
> In this case, the previous logic was that with_bundled_wheel is yes if without_bundled_wheel=no and enable_bundled=yes. The new logic is that if enable_bundled_wheel is yes if enable_bundled_wheel=yes and enable_bundled=yes, however if enable_bundled=no and enable_budled_wheel=yes, we still set WITH_BUNDLED_WHEEL to true, which is wrong.
>
> I *believe* that the intention of --disable-bundled is to disable all bundling, to for each package, it should only be bundled if enable_bundled=yes and enable_bundled_$PACKAGE=yes. I'll review and update to apply this logic consistently across Mesos and Libprocess.
I posted a second commit in https://reviews.apache.org/r/35084/ to address the consistency of using bundled packages.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33752/#review82272
-----------------------------------------------------------
On June 4, 2015, 7:31 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33752/
> -----------------------------------------------------------
>
> (Updated June 4, 2015, 7:31 p.m.)
>
>
> Review request for mesos, Cody Maloney and Timothy St. Clair.
>
>
> Bugs: MESOS-2537
> https://issues.apache.org/jira/browse/MESOS-2537
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In a number of places, the Mesos configure script passes "$foo=yes"
> to the 2nd argument of AC_ARG_ENABLED. However, the 2nd argument
> is invoked when the option is provided in any form, not just when
> the --enable-foo form is used. One result of this is that
> --disable-optimize doesn't work.
>
> The correct handling of the 2nd argument is to save the value of
> "$enableval". This change sets the value of all the enable variables
> using $enableval, and sets the default value based on the option
> name.
>
> There are a number of enable options that were internally named
> "$with_foo" and "$without_foo". Rename these to "$enable_foo" for
> clarity and to remove the need for both a with_ and a without_
> version.
>
> Finally, emit the compilation flags at the end of the configure
> phase so it is easier to see the results of your configuration
> options.
>
>
> Diffs
> -----
>
> configure.ac cad7f0e92eacc86d37b3f578382946db8b466531
>
> Diff: https://reviews.apache.org/r/33752/diff/
>
>
> Testing
> -------
>
> Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify that the status summary reflects the expected compiler flags. Verify that --enable-foo and --disable-foo do different things.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 33752: MESOS-2537: Fix AC_ARG_ENABLED option
processing
Posted by James Peach <jp...@apache.org>.
> On May 1, 2015, 7:30 p.m., Cody Maloney wrote:
> > configure.ac, line 1407
> > <https://reviews.apache.org/r/33752/diff/1/?file=947254#file947254line1407>
> >
> > I worry this changes the behavior some when --disable-bundled is set (Previously it seems like it would see if it could find the package locally, and if not enable the bundled one). Internally we don't really use that codepath though.
In this case, the previous logic was that with_bundled_wheel is yes if without_bundled_wheel=no and enable_bundled=yes. The new logic is that if enable_bundled_wheel is yes if enable_bundled_wheel=yes and enable_bundled=yes, however if enable_bundled=no and enable_budled_wheel=yes, we still set WITH_BUNDLED_WHEEL to true, which is wrong.
I *believe* that the intention of --disable-bundled is to disable all bundling, to for each package, it should only be bundled if enable_bundled=yes and enable_bundled_$PACKAGE=yes. I'll review and update to apply this logic consistently across Mesos and Libprocess.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33752/#review82272
-----------------------------------------------------------
On May 1, 2015, 7:51 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33752/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 7:51 p.m.)
>
>
> Review request for mesos, Cody Maloney and Timothy St. Clair.
>
>
> Bugs: MESOS-2537
> https://issues.apache.org/jira/browse/MESOS-2537
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In a number of places, the Mesos configure script passes "$foo=yes"
> to the 2nd argument of AC_ARG_ENABLED. However, the 2nd argument
> is invoked when the option is provided in any form, not just when
> the --enable-foo form is used. One result of this is that
> --disable-optimize doesn't work.
>
> The correct handling of the 2nd argument is to save the value of
> "$enableval". This change sets the value of all the enable variables
> using $enableval, and sets the default value based on the option
> name.
>
> There are a number of enable options that were internally named
> "$with_foo" and "$without_foo". Rename these to "$enable_foo" for
> clarity and to remove the need for both a with_ and a without_
> version.
>
> Finally, emit the compilation flags at the end of the configure
> phase so it is easier to see the results of your configuration
> options.
>
>
> Diffs
> -----
>
> configure.ac 589ae97d0432370b462576cd1985544564893999
>
> Diff: https://reviews.apache.org/r/33752/diff/
>
>
> Testing
> -------
>
> Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify that the status summary reflects the expected compiler flags. Verify that --enable-foo and --disable-foo do different things.
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 33752: MESOS-2537: Fix AC_ARG_ENABLED option
processing
Posted by Cody Maloney <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33752/#review82272
-----------------------------------------------------------
Overall looks good to me. Should have Tim St. Clair [~tstclair] review as he's the autotools guru.
configure.ac
<https://reviews.apache.org/r/33752/#comment132994>
I worry this changes the behavior some when --disable-bundled is set (Previously it seems like it would see if it could find the package locally, and if not enable the bundled one). Internally we don't really use that codepath though.
- Cody Maloney
On May 1, 2015, 3:42 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33752/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 3:42 p.m.)
>
>
> Review request for mesos and Cody Maloney.
>
>
> Bugs: MESOS-2537
> https://issues.apache.org/jira/browse/MESOS-2537
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In a number of places, the Mesos configure script passes "$foo=yes"
> to the 2nd argument of AC_ARG_ENABLED. However, the 2nd argument
> is invoked when the option is provided in any form, not just when
> the --enable-foo form is used. One result of this is that
> --disable-optimize doesn't work.
>
> The correct handling of the 2nd argument is to save the value of
> "$enableval". This change sets the value of all the enable variables
> using $enableval, and sets the default value based on the option
> name.
>
> There are a number of enable options that were internally named
> "$with_foo" and "$without_foo". Rename these to "$enable_foo" for
> clarity and to remove the need for both a with_ and a without_
> version.
>
> Finally, emit the compilation flags at the end of the configure
> phase so it is easier to see the results of your configuration
> options.
>
>
> Diffs
> -----
>
> configure.ac 589ae97d0432370b462576cd1985544564893999
>
> Diff: https://reviews.apache.org/r/33752/diff/
>
>
> Testing
> -------
>
> Configure and build on CentOS 7 and Mac OS X 10.10.3. Verify that the status summary reflects the expected compiler flags. Verify that --enable-foo and --disable-foo do different things.
>
>
> Thanks,
>
> James Peach
>
>