Re: Review Request 33752: MESOS-2537: Fix AC_ARG_ENABLED option processing

2016-10-21 Thread Benjamin Bannier 

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



It would be great to get MESOS-2537 fixed, would you consider bringing this 
chain up to date @jpeach?

If it helps I would be happy to help reviewing changes after 2016-10-31.


configure.ac (line 1584)


Might be clearer to add this in a separate patch.


- Benjamin Bannier


On Aug. 26, 2015, 10:11 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33752/
> ---
> 
> (Updated Aug. 26, 2015, 10:11 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 87461d73ed04c4cf176c3475ded9f98dadcda608 
> 
> 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

2016-07-09 Thread Joris Van Remoortere 

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On Aug. 26, 2015, 8:11 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33752/
> ---
> 
> (Updated Aug. 26, 2015, 8:11 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 87461d73ed04c4cf176c3475ded9f98dadcda608 
> 
> 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

2015-08-26 Thread James Peach 

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

(Updated Aug. 26, 2015, 8:11 p.m.)


Review request for mesos, Cody Maloney and Timothy St. Clair.


Changes
---

Rebased.


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 (updated)
-

  configure.ac 87461d73ed04c4cf176c3475ded9f98dadcda608 

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

2015-08-26 Thread Timothy St. Clair 

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



configure.ac (line 145)
https://reviews.apache.org/r/33752/#comment152174

doesn't this change default toggle to an evaluation of the output variable? 
 

Shouldn't this just be a toggle on existance?  

I'm ok on uses of $withval but do you have an arguement for the enableval?


- Timothy St. Clair


On Aug. 26, 2015, 8:11 p.m., James Peach wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/33752/
 ---
 
 (Updated Aug. 26, 2015, 8:11 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 87461d73ed04c4cf176c3475ded9f98dadcda608 
 
 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

2015-08-26 Thread James Peach 


 On Aug. 26, 2015, 9:01 p.m., Timothy St. Clair wrote:
 
 
 Timothy St. Clair wrote:
 Have you tested all combinations here?

No, only some of the possible combinations. The addition of buld flags output 
makes is easier to tell when it it doing the right thing. This change doesn't 
really change any logic except for ```--enable-debug``` and 
```--enable-optimize```, since the absence of defaults for those is confusing.


 On Aug. 26, 2015, 9:01 p.m., Timothy St. Clair wrote:
  configure.ac, line 145
  https://reviews.apache.org/r/33752/diff/5/?file=1055235#file1055235line145
 
  doesn't this change default toggle to an evaluation of the output 
  variable?  
  
  Shouldn't this just be a toggle on existance?  
  
  I'm ok on uses of $withval but do you have an arguement for the 
  enableval?

The ```--enable-foo``` syntax also supports ```--disable-foo```. So if the 
build option is not specified, the default should apply, otherwise the operator 
can specify the build option must be present or absent. For example, our 
internal RPM build was specifying ```--disable-perftools```, but this had the 
unintended effect of enabling perftools.


- James


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


On Aug. 26, 2015, 8:11 p.m., James Peach wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/33752/
 ---
 
 (Updated Aug. 26, 2015, 8:11 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 87461d73ed04c4cf176c3475ded9f98dadcda608 
 
 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

2015-08-24 Thread James Peach 

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

(Updated Aug. 24, 2015, 9:54 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 (updated)
-

  configure.ac db0632d60a6d82ab396931b4d913f34b625a45a7 

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

2015-06-08 Thread James Peach 

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

(Updated June 9, 2015, 12:02 a.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 (updated)
-

  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

2015-06-04 Thread James Peach 


 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

2015-06-04 Thread James Peach 


 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

2015-05-01 Thread Mesos ReviewBot 

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