Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2016-02-04 Thread Till Toenshoff 

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


Fix it, then Ship it!




Thanks James! There are two minor style nits left, will fix while committing 
but will leave them open for your information. Please close them once you 
skimmed over them :)


src/tests/module_tests.cpp (line 66)


Thanks for this one :)



src/tests/module_tests.cpp (lines 128 - 129)


The correct wrapping here would be:

```
  library->set_file(
  path::join(libraryDirectory, os::libraries::expandName(libraryName)));
```

It is an argument continuation, hence we indent by 4 spaces.



src/tests/module_tests.cpp (lines 160 - 161)


The correct wrapping here would be:

```
  string libraryFile =
path::join(libraryDirectory, os::libraries::expandName(libraryName));
```

Note how we prefer to wrap at assignments if possible. Then given that this 
is not an argument wrapping, we now indent by 2 spaces.


- Till Toenshoff


On Jan. 27, 2016, 6:44 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39781/
> ---
> 
> (Updated Jan. 27, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-3725
> https://issues.apache.org/jira/browse/MESOS-3725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update ModuleTest to not assume dynamic dlopen search.
> 
> 
> Diffs
> -
> 
>   src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 
> 
> Diff: https://reviews.apache.org/r/39781/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2016-01-27 Thread James Peach 

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

(Updated Jan. 27, 2016, 6:44 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Changes
---

Updated.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2016-01-26 Thread James Peach 

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

(Updated Jan. 26, 2016, 7:24 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Changes
---

Rebased onto master.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2016-01-26 Thread Benjamin Bannier 

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


Fix it, then Ship it!





src/tests/module_tests.cpp (line 167)


Drop unnecessary parentheses.


- Benjamin Bannier


On Jan. 26, 2016, 8:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39781/
> ---
> 
> (Updated Jan. 26, 2016, 8:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-3725
> https://issues.apache.org/jira/browse/MESOS-3725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update ModuleTest to not assume dynamic dlopen search.
> 
> 
> Diffs
> -
> 
>   src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 
> 
> Diff: https://reviews.apache.org/r/39781/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2016-01-15 Thread James Peach 

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

(Updated Jan. 16, 2016, 12:45 a.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Changes
---

Rebased onto master.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2016-01-12 Thread James Peach 

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

(Updated Jan. 12, 2016, 10:14 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Changes
---

Rebased onto master.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2016-01-11 Thread James Peach 

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

(Updated Jan. 11, 2016, 10:32 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Changes
---

Rebase onto master.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2016-01-08 Thread Benjamin Bannier 


> On Nov. 30, 2015, 9:51 a.m., Benjamin Bannier wrote:
> > src/tests/module_tests.cpp, line 123
> > 
> >
> > None of these functions need to be (mutating!) member functions if you 
> > simply inject `libraryDirectory`; please consider converting them to free 
> > functions, e.g. implemented in an anon namespace in this translation unit.
> 
> James Peach wrote:
> What exactly do you mean by "inject"? Pass the ```libraryDirectory``` as 
> a parameter to these free functions? Or use some Google Test or Mock 
> injection technique?
> 
> I'd prefer not to clutter this change with additional refactorings if 
> that's OK.

I meant the former, but member functions are good enough too I guess. Since you 
changed these from `static` functions to methods could you please at least make 
them `const` so it is clear that they are not mutating instance data, though?


- Benjamin


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


On Dec. 16, 2015, 9:17 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39781/
> ---
> 
> (Updated Dec. 16, 2015, 9:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-3725
> https://issues.apache.org/jira/browse/MESOS-3725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update ModuleTest to not assume dynamic dlopen search.
> 
> 
> Diffs
> -
> 
>   src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 
> 
> Diff: https://reviews.apache.org/r/39781/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-12-16 Thread James Peach 

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

(Updated Dec. 16, 2015, 8:17 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Changes
---

Rebased onto master.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-12-10 Thread James Peach 

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

(Updated Dec. 10, 2015, 6:45 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Changes
---

Rebased onto master.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-12-08 Thread James Peach 

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

(Updated Dec. 8, 2015, 6:52 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-12-08 Thread James Peach 

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

(Updated Dec. 8, 2015, 8:58 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-12-08 Thread James Peach 


> On Nov. 30, 2015, 8:51 a.m., Benjamin Bannier wrote:
> > src/tests/module_tests.cpp, line 123
> > 
> >
> > None of these functions need to be (mutating!) member functions if you 
> > simply inject `libraryDirectory`; please consider converting them to free 
> > functions, e.g. implemented in an anon namespace in this translation unit.

What exactly do you mean by "inject"? Pass the ```libraryDirectory``` as a 
parameter to these free functions? Or use some Google Test or Mock injection 
technique?

I'd prefer not to clutter this change with additional refactorings if that's OK.


- James


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


On Dec. 8, 2015, 6:52 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39781/
> ---
> 
> (Updated Dec. 8, 2015, 6:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-3725
> https://issues.apache.org/jira/browse/MESOS-3725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update ModuleTest to not assume dynamic dlopen search.
> 
> 
> Diffs
> -
> 
>   src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 
> 
> Diff: https://reviews.apache.org/r/39781/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-12-07 Thread James Peach 

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

(Updated Dec. 7, 2015, 5:17 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp b5cfbcd4885a4f2c20b19e00958fedda81afa6e0 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-12-07 Thread Niklas Nielsen 


> On Nov. 30, 2015, 12:51 a.m., Benjamin Bannier wrote:
> >

James - are you blocked by anything? Can we help? It looks like we need one 
more review iteration.


- Niklas


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


On Nov. 26, 2015, 6:04 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39781/
> ---
> 
> (Updated Nov. 26, 2015, 6:04 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-3725
> https://issues.apache.org/jira/browse/MESOS-3725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update ModuleTest to not assume dynamic dlopen search.
> 
> 
> Diffs
> -
> 
>   src/tests/module_tests.cpp 28c71b0c1960bad4933f86d35fe8a0248fef326e 
> 
> Diff: https://reviews.apache.org/r/39781/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-11-30 Thread Benjamin Bannier 

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



src/tests/module_tests.cpp (line 54)


Since there is only one item left, this needs to be reworded, e.g.

> .., we do th `dlopen()` the examplemodule library and ..



src/tests/module_tests.cpp (line 70)


Not yours, but this should really be a `static_cast`.



src/tests/module_tests.cpp (line 77)


Since `TearDownTestCase` is effectively a fixture's dtr, both zeroing out 
`moduleBase` (a raw ptr) and clearing `libraryDirectory` (just a `string`) do 
nothing; please consider dropping.



src/tests/module_tests.cpp (line 112)


None of these functions need to be (mutating!) member functions if you 
simply inject `libraryDirectory`; please consider converting them to free 
functions, e.g. implemented in an anon namespace in this translation unit.



src/tests/module_tests.cpp 


Good catch! Looks like we already call `expandName` in most places.


- Benjamin Bannier


On Nov. 26, 2015, 2:04 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39781/
> ---
> 
> (Updated Nov. 26, 2015, 2:04 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-3725
> https://issues.apache.org/jira/browse/MESOS-3725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update ModuleTest to not assume dynamic dlopen search.
> 
> 
> Diffs
> -
> 
>   src/tests/module_tests.cpp 28c71b0c1960bad4933f86d35fe8a0248fef326e 
> 
> Diff: https://reviews.apache.org/r/39781/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-11-19 Thread James Peach 

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

(Updated Nov. 20, 2015, 1:27 a.m.)


Review request for mesos, Kapil Arya and Niklas Nielsen.


Changes
---

Rebase and fix formatting warnings.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp 28c71b0c1960bad4933f86d35fe8a0248fef326e 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-11-18 Thread James Peach 

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

(Updated Nov. 19, 2015, 12:48 a.m.)


Review request for mesos.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp 28c71b0c1960bad4933f86d35fe8a0248fef326e 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach

Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

2015-10-29 Thread James Peach 

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

Review request for mesos.


Bugs: MESOS-3725
https://issues.apache.org/jira/browse/MESOS-3725


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs
-

  src/tests/module_tests.cpp 60497aac3200ab9a679a81a593b5bf0d02fd4b50 

Diff: https://reviews.apache.org/r/39781/diff/


Testing
---

make check


Thanks,

James Peach