Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 16, 2017, 11:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 16, 2017, 11:43 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac ee3818d404013e172bc51f11e8c5792cb335a22c 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/5/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[Qian Zhang]

> On July 17, 2017, 11:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > 
> >
> > Mind to elaborate a bit about why introducing a new .m4 file? What 
> > about just introducing a new configure option (like `--enable-libnl3`) and 
> > make `--enable_port_mapping_isolator` depend on it? Just like the 
> > relationship between `--enable-ssl` and `--enable-libevent`: 
> > https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698
> > 
> > And for `--enable-libnl3`, I think we should not do much checks, just 
> > some very basic check should be OK, and on top of it, 
> > `--enable_port_mapping_isolator` can do some more advanced/specific check 
> > on libnl3 for itself.
> 
> James Peach wrote:
> > Mind to elaborate a bit about why introducing a new .m4 file?
> 
> This is because a separate file makes it easier to maintain. We could 
> inline the macro into `configure.ac` but that's not typical autotool practice.
> 
> > What about just introducing a new configure option (like 
> --enable-libnl3)
> 
> I don't see how `--enable-libnl3` makes anything simpler, unless you want 
> to to use that option to enable all the features that depend on `libnl3`. In 
> that case, however, I'd just implement that based on the existing `--with-nl`.
> 
> The semantics here are that we have `--enable-port-mapping-isolator`, 
> `--enable-network-ports-isolator` and `--with-nl` arguments. If either of the 
> first 2 options is enabled, we will attempt to use the `libnl3` from the 
> location of the 3rd option.
> 
> > I think we should not do much checks
> 
> The checks we are applying are all ones that we already had. I didn't 
> want to change the dependency requirements here.
> 
> Qian Zhang wrote:
> I was thinking to use `--enable-libnl3` to check basic libnl3 features, 
> and use `--enable_port_mapping_isolator` and 
> `--enable-network-ports-isolator` to check the advanced libnl3 features 
> needed by them respectively.
> 
> So in your solution, there will be only 2 `--enable-xxx` flags (i.e., 
> `--enable_port_mapping_isolator`, `--enable-network-ports-isolator`) and 
> `--with-nl`, and `MESOS_HAVE_LIBNL3` is internal which is not exposed to the 
> user, right?
> 
> James Peach wrote:
> > I was thinking to use --enable-libnl3 to check basic libnl3 features, 
> and use --enable_port_mapping_isolator and --enable-network-ports-isolator to 
> check the advanced libnl3 features needed by them respectively.
> 
> At this point, I'd prefer not to change the semantics of this. We have a 
> pretty clear, documented requirement for libnl >= 3.2.6 and if we start 
> accepting different versions, it becomes less clear.
> 
> > So in your solution, there will be only 2 --enable-xxx flags (i.e., 
> --enable_port_mapping_isolator, --enable-network-ports-isolator) and 
> --with-nl, 
> 
> Yes, that's correct.
> 
> > and MESOS_HAVE_LIBNL3 is internal which is not exposed to the user, 
> right?
> 
> `MESOS_HAVE_LIBNL3` is just the autoconf macro name, so it's not exposed 
> either to the user or to the build. There is a new build conditional named 
> `ENABLE_LINUX_ROUTING` which causes the `src/linux/routing` files to be 
> included in the build
> 
> Qian Zhang wrote:
> Can you please let me know where `ENABLE_LINUX_ROUTING` is? I do not see 
> it in this patch and in the current Mesos code.
> 
> James Peach wrote:
> It gets introduced in the next patch 
> [r/60492](https://reviews.apache.org/r/60492).

I remember Jie had some comments regarding libnl when we were working on 
MESOS-5646, I will ask him to check if he has some comments on this patch.


- Qian


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


On July 17, 2017, 7:43 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 17, 2017, 7:43 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of 

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[James Peach]

> On July 17, 2017, 3:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > 
> >
> > Mind to elaborate a bit about why introducing a new .m4 file? What 
> > about just introducing a new configure option (like `--enable-libnl3`) and 
> > make `--enable_port_mapping_isolator` depend on it? Just like the 
> > relationship between `--enable-ssl` and `--enable-libevent`: 
> > https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698
> > 
> > And for `--enable-libnl3`, I think we should not do much checks, just 
> > some very basic check should be OK, and on top of it, 
> > `--enable_port_mapping_isolator` can do some more advanced/specific check 
> > on libnl3 for itself.
> 
> James Peach wrote:
> > Mind to elaborate a bit about why introducing a new .m4 file?
> 
> This is because a separate file makes it easier to maintain. We could 
> inline the macro into `configure.ac` but that's not typical autotool practice.
> 
> > What about just introducing a new configure option (like 
> --enable-libnl3)
> 
> I don't see how `--enable-libnl3` makes anything simpler, unless you want 
> to to use that option to enable all the features that depend on `libnl3`. In 
> that case, however, I'd just implement that based on the existing `--with-nl`.
> 
> The semantics here are that we have `--enable-port-mapping-isolator`, 
> `--enable-network-ports-isolator` and `--with-nl` arguments. If either of the 
> first 2 options is enabled, we will attempt to use the `libnl3` from the 
> location of the 3rd option.
> 
> > I think we should not do much checks
> 
> The checks we are applying are all ones that we already had. I didn't 
> want to change the dependency requirements here.
> 
> Qian Zhang wrote:
> I was thinking to use `--enable-libnl3` to check basic libnl3 features, 
> and use `--enable_port_mapping_isolator` and 
> `--enable-network-ports-isolator` to check the advanced libnl3 features 
> needed by them respectively.
> 
> So in your solution, there will be only 2 `--enable-xxx` flags (i.e., 
> `--enable_port_mapping_isolator`, `--enable-network-ports-isolator`) and 
> `--with-nl`, and `MESOS_HAVE_LIBNL3` is internal which is not exposed to the 
> user, right?
> 
> James Peach wrote:
> > I was thinking to use --enable-libnl3 to check basic libnl3 features, 
> and use --enable_port_mapping_isolator and --enable-network-ports-isolator to 
> check the advanced libnl3 features needed by them respectively.
> 
> At this point, I'd prefer not to change the semantics of this. We have a 
> pretty clear, documented requirement for libnl >= 3.2.6 and if we start 
> accepting different versions, it becomes less clear.
> 
> > So in your solution, there will be only 2 --enable-xxx flags (i.e., 
> --enable_port_mapping_isolator, --enable-network-ports-isolator) and 
> --with-nl, 
> 
> Yes, that's correct.
> 
> > and MESOS_HAVE_LIBNL3 is internal which is not exposed to the user, 
> right?
> 
> `MESOS_HAVE_LIBNL3` is just the autoconf macro name, so it's not exposed 
> either to the user or to the build. There is a new build conditional named 
> `ENABLE_LINUX_ROUTING` which causes the `src/linux/routing` files to be 
> included in the build
> 
> Qian Zhang wrote:
> Can you please let me know where `ENABLE_LINUX_ROUTING` is? I do not see 
> it in this patch and in the current Mesos code.

It gets introduced in the next patch 
[r/60492](https://reviews.apache.org/r/60492).


- James


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


On July 16, 2017, 11:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 16, 2017, 11:43 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[Qian Zhang]

> On July 17, 2017, 11:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > 
> >
> > Mind to elaborate a bit about why introducing a new .m4 file? What 
> > about just introducing a new configure option (like `--enable-libnl3`) and 
> > make `--enable_port_mapping_isolator` depend on it? Just like the 
> > relationship between `--enable-ssl` and `--enable-libevent`: 
> > https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698
> > 
> > And for `--enable-libnl3`, I think we should not do much checks, just 
> > some very basic check should be OK, and on top of it, 
> > `--enable_port_mapping_isolator` can do some more advanced/specific check 
> > on libnl3 for itself.
> 
> James Peach wrote:
> > Mind to elaborate a bit about why introducing a new .m4 file?
> 
> This is because a separate file makes it easier to maintain. We could 
> inline the macro into `configure.ac` but that's not typical autotool practice.
> 
> > What about just introducing a new configure option (like 
> --enable-libnl3)
> 
> I don't see how `--enable-libnl3` makes anything simpler, unless you want 
> to to use that option to enable all the features that depend on `libnl3`. In 
> that case, however, I'd just implement that based on the existing `--with-nl`.
> 
> The semantics here are that we have `--enable-port-mapping-isolator`, 
> `--enable-network-ports-isolator` and `--with-nl` arguments. If either of the 
> first 2 options is enabled, we will attempt to use the `libnl3` from the 
> location of the 3rd option.
> 
> > I think we should not do much checks
> 
> The checks we are applying are all ones that we already had. I didn't 
> want to change the dependency requirements here.
> 
> Qian Zhang wrote:
> I was thinking to use `--enable-libnl3` to check basic libnl3 features, 
> and use `--enable_port_mapping_isolator` and 
> `--enable-network-ports-isolator` to check the advanced libnl3 features 
> needed by them respectively.
> 
> So in your solution, there will be only 2 `--enable-xxx` flags (i.e., 
> `--enable_port_mapping_isolator`, `--enable-network-ports-isolator`) and 
> `--with-nl`, and `MESOS_HAVE_LIBNL3` is internal which is not exposed to the 
> user, right?
> 
> James Peach wrote:
> > I was thinking to use --enable-libnl3 to check basic libnl3 features, 
> and use --enable_port_mapping_isolator and --enable-network-ports-isolator to 
> check the advanced libnl3 features needed by them respectively.
> 
> At this point, I'd prefer not to change the semantics of this. We have a 
> pretty clear, documented requirement for libnl >= 3.2.6 and if we start 
> accepting different versions, it becomes less clear.
> 
> > So in your solution, there will be only 2 --enable-xxx flags (i.e., 
> --enable_port_mapping_isolator, --enable-network-ports-isolator) and 
> --with-nl, 
> 
> Yes, that's correct.
> 
> > and MESOS_HAVE_LIBNL3 is internal which is not exposed to the user, 
> right?
> 
> `MESOS_HAVE_LIBNL3` is just the autoconf macro name, so it's not exposed 
> either to the user or to the build. There is a new build conditional named 
> `ENABLE_LINUX_ROUTING` which causes the `src/linux/routing` files to be 
> included in the build

Can you please let me know where `ENABLE_LINUX_ROUTING` is? I do not see it in 
this patch and in the current Mesos code.


- Qian


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


On July 17, 2017, 7:43 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 17, 2017, 7:43 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[James Peach]

> On July 17, 2017, 3:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > 
> >
> > Mind to elaborate a bit about why introducing a new .m4 file? What 
> > about just introducing a new configure option (like `--enable-libnl3`) and 
> > make `--enable_port_mapping_isolator` depend on it? Just like the 
> > relationship between `--enable-ssl` and `--enable-libevent`: 
> > https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698
> > 
> > And for `--enable-libnl3`, I think we should not do much checks, just 
> > some very basic check should be OK, and on top of it, 
> > `--enable_port_mapping_isolator` can do some more advanced/specific check 
> > on libnl3 for itself.
> 
> James Peach wrote:
> > Mind to elaborate a bit about why introducing a new .m4 file?
> 
> This is because a separate file makes it easier to maintain. We could 
> inline the macro into `configure.ac` but that's not typical autotool practice.
> 
> > What about just introducing a new configure option (like 
> --enable-libnl3)
> 
> I don't see how `--enable-libnl3` makes anything simpler, unless you want 
> to to use that option to enable all the features that depend on `libnl3`. In 
> that case, however, I'd just implement that based on the existing `--with-nl`.
> 
> The semantics here are that we have `--enable-port-mapping-isolator`, 
> `--enable-network-ports-isolator` and `--with-nl` arguments. If either of the 
> first 2 options is enabled, we will attempt to use the `libnl3` from the 
> location of the 3rd option.
> 
> > I think we should not do much checks
> 
> The checks we are applying are all ones that we already had. I didn't 
> want to change the dependency requirements here.
> 
> Qian Zhang wrote:
> I was thinking to use `--enable-libnl3` to check basic libnl3 features, 
> and use `--enable_port_mapping_isolator` and 
> `--enable-network-ports-isolator` to check the advanced libnl3 features 
> needed by them respectively.
> 
> So in your solution, there will be only 2 `--enable-xxx` flags (i.e., 
> `--enable_port_mapping_isolator`, `--enable-network-ports-isolator`) and 
> `--with-nl`, and `MESOS_HAVE_LIBNL3` is internal which is not exposed to the 
> user, right?

> I was thinking to use --enable-libnl3 to check basic libnl3 features, and use 
> --enable_port_mapping_isolator and --enable-network-ports-isolator to check 
> the advanced libnl3 features needed by them respectively.

At this point, I'd prefer not to change the semantics of this. We have a pretty 
clear, documented requirement for libnl >= 3.2.6 and if we start accepting 
different versions, it becomes less clear.

> So in your solution, there will be only 2 --enable-xxx flags (i.e., 
> --enable_port_mapping_isolator, --enable-network-ports-isolator) and 
> --with-nl, 

Yes, that's correct.

> and MESOS_HAVE_LIBNL3 is internal which is not exposed to the user, right?

`MESOS_HAVE_LIBNL3` is just the autoconf macro name, so it's not exposed either 
to the user or to the build. There is a new build conditional named 
`ENABLE_LINUX_ROUTING` which causes the `src/linux/routing` files to be 
included in the build


- James


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


On July 16, 2017, 11:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 16, 2017, 11:43 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[Qian Zhang]

> On July 17, 2017, 11:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > 
> >
> > Mind to elaborate a bit about why introducing a new .m4 file? What 
> > about just introducing a new configure option (like `--enable-libnl3`) and 
> > make `--enable_port_mapping_isolator` depend on it? Just like the 
> > relationship between `--enable-ssl` and `--enable-libevent`: 
> > https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698
> > 
> > And for `--enable-libnl3`, I think we should not do much checks, just 
> > some very basic check should be OK, and on top of it, 
> > `--enable_port_mapping_isolator` can do some more advanced/specific check 
> > on libnl3 for itself.
> 
> James Peach wrote:
> > Mind to elaborate a bit about why introducing a new .m4 file?
> 
> This is because a separate file makes it easier to maintain. We could 
> inline the macro into `configure.ac` but that's not typical autotool practice.
> 
> > What about just introducing a new configure option (like 
> --enable-libnl3)
> 
> I don't see how `--enable-libnl3` makes anything simpler, unless you want 
> to to use that option to enable all the features that depend on `libnl3`. In 
> that case, however, I'd just implement that based on the existing `--with-nl`.
> 
> The semantics here are that we have `--enable-port-mapping-isolator`, 
> `--enable-network-ports-isolator` and `--with-nl` arguments. If either of the 
> first 2 options is enabled, we will attempt to use the `libnl3` from the 
> location of the 3rd option.
> 
> > I think we should not do much checks
> 
> The checks we are applying are all ones that we already had. I didn't 
> want to change the dependency requirements here.

I was thinking to use `--enable-libnl3` to check basic libnl3 features, and use 
`--enable_port_mapping_isolator` and `--enable-network-ports-isolator` to check 
the advanced libnl3 features needed by them respectively.

So in your solution, there will be only 2 `--enable-xxx` flags (i.e., 
`--enable_port_mapping_isolator`, `--enable-network-ports-isolator`) and 
`--with-nl`, and `MESOS_HAVE_LIBNL3` is internal which is not exposed to the 
user, right?


- Qian


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


On July 17, 2017, 7:43 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 17, 2017, 7:43 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[James Peach]

> On July 17, 2017, 5:14 a.m., Qian Zhang wrote:
> > Do we need to change anything in CMake side?

No, this isn't implemented in the CMake build.


- James


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


On July 16, 2017, 11:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 16, 2017, 11:43 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[Qian Zhang]

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



Do we need to change anything in CMake side?

- Qian Zhang


On July 17, 2017, 7:43 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 17, 2017, 7:43 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[James Peach]

> On July 17, 2017, 3:20 a.m., Qian Zhang wrote:
> > configure.ac
> > Line 350 (original)
> > 
> >
> > So `--with-nl` is removed, will it cause backward compatibility issue?

No, `--with-nl` is provided by the `MESOS_HAVE_LIBNL3` macro.


> On July 17, 2017, 3:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > 
> >
> > Mind to elaborate a bit about why introducing a new .m4 file? What 
> > about just introducing a new configure option (like `--enable-libnl3`) and 
> > make `--enable_port_mapping_isolator` depend on it? Just like the 
> > relationship between `--enable-ssl` and `--enable-libevent`: 
> > https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698
> > 
> > And for `--enable-libnl3`, I think we should not do much checks, just 
> > some very basic check should be OK, and on top of it, 
> > `--enable_port_mapping_isolator` can do some more advanced/specific check 
> > on libnl3 for itself.

> Mind to elaborate a bit about why introducing a new .m4 file?

This is because a separate file makes it easier to maintain. We could inline 
the macro into `configure.ac` but that's not typical autotool practice.

> What about just introducing a new configure option (like --enable-libnl3)

I don't see how `--enable-libnl3` makes anything simpler, unless you want to to 
use that option to enable all the features that depend on `libnl3`. In that 
case, however, I'd just implement that based on the existing `--with-nl`.

The semantics here are that we have `--enable-port-mapping-isolator`, 
`--enable-network-ports-isolator` and `--with-nl` arguments. If either of the 
first 2 options is enabled, we will attempt to use the `libnl3` from the 
location of the 3rd option.

> I think we should not do much checks

The checks we are applying are all ones that we already had. I didn't want to 
change the dependency requirements here.


- James


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


On July 16, 2017, 11:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 16, 2017, 11:43 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[Qian Zhang]

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




configure.ac
Line 350 (original)


So `--with-nl` is removed, will it cause backward compatibility issue?



configure.ac
Line 1297 (original), 1285 (patched)


Do you want to change `network isolator` to `port mapping isolator` too?



m4/libnl3.m4
Lines 33-37 (patched)


Mind to elaborate a bit about why introducing a new .m4 file? What about 
just introducing a new configure option (like `--enable-libnl3`) and make 
`--enable_port_mapping_isolator` depend on it? Just like the relationship 
between `--enable-ssl` and `--enable-libevent`: 
https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698

And for `--enable-libnl3`, I think we should not do much checks, just some 
very basic check should be OK, and on top of it, 
`--enable_port_mapping_isolator` can do some more advanced/specific check on 
libnl3 for itself.


- Qian Zhang


On July 17, 2017, 7:43 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 17, 2017, 7:43 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>