mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: Review Request 60902: Moved the libnl3 configure checks into a macro.
Date Mon, 17 Jul 2017 23:41:32 GMT


> On July 17, 2017, 3:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > <https://reviews.apache.org/r/60902/diff/1/?file=1777351#file1777351line33>
> >
> >     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
> 
>


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