Review Request 45940: Agent:[1/2] Fixed transitive #include problem in `load.cpp`.

[Alex Clemmer <cl...@gmail.com>]

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

Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
-------

`qos_controllers/load.cpp` currently includes `stout/posix/os.hpp`
directly, which includes `killtree`, which includes `os.hpp`. When we
move `os::libraries` back to `os.hpp`, this will cause a problem,
because it will mean that `os::setenv` (which is defined further down in
`stout/posix/os.hpp` is not defined before
`os::libraries::Library::setPaths`, which uses it.

In general, this problem is usually caused by including something like
`stout/posix/os.hpp` instead of `os.hpp`. The choice to include such a
header is somewhat fraught, because many files in `os.hpp` (particularly
those that are included into `os.hpp` via a derivative, like
`posix/os.hpp`) have implicit dependencies on functions they `#include`
being defined before their code is defined. This issue will probably get
worse in the future. Furthermore, it is worth noting that the plan is
to, at some point, remove `os.hpp` altogether. Depending on the specific
implementation, this could make the "transitive #include problem" either
dramatically worse, or much better. This is a discussion the community
needs to have more braodly.

This commit will resovle the compile problem by removing `load.cpp`'s
explicit dependency on `os/os.hpp` and `posix/os.hpp`, and replacing
them with plain-old `stout/os.hpp`. As noted, however, this is really an
immediate solution to a larger problem, which we don't attempt to fully
address here.


Diffs
-----

  src/slave/qos_controllers/load.cpp dd44f9209ad283bfea95f16a8c1017e309757f23 

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


Testing
-------


Thanks,

Alex Clemmer

Re: Review Request 45940: Agent:[1/2] Fixed transitive #include problem in `load.cpp`.

[Michael Park <mp...@apache.org>]

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


Ship it!




Ship It!

- Michael Park


On April 8, 2016, 7:59 p.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45940/
> -----------------------------------------------------------
> 
> (Updated April 8, 2016, 7:59 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> `qos_controllers/load.cpp` currently includes `stout/posix/os.hpp`
> directly, which includes `killtree`, which includes `os.hpp`. When we
> move `os::libraries` back to `os.hpp`, this will cause a problem,
> because it will mean that `os::setenv` (which is defined further down in
> `stout/posix/os.hpp` is not defined before
> `os::libraries::Library::setPaths`, which uses it.
> 
> In general, this problem is usually caused by including something like
> `stout/posix/os.hpp` instead of `os.hpp`. The choice to include such a
> header is somewhat fraught, because many files in `os.hpp` (particularly
> those that are included into `os.hpp` via a derivative, like
> `posix/os.hpp`) have implicit dependencies on functions they `#include`
> being defined before their code is defined. This issue will probably get
> worse in the future. Furthermore, it is worth noting that the plan is
> to, at some point, remove `os.hpp` altogether. Depending on the specific
> implementation, this could make the "transitive #include problem" either
> dramatically worse, or much better. This is a discussion the community
> needs to have more braodly.
> 
> This commit will resovle the compile problem by removing `load.cpp`'s
> explicit dependency on `os/os.hpp` and `posix/os.hpp`, and replacing
> them with plain-old `stout/os.hpp`. As noted, however, this is really an
> immediate solution to a larger problem, which we don't attempt to fully
> address here.
> 
> 
> Diffs
> -----
> 
>   src/slave/qos_controllers/load.cpp dd44f9209ad283bfea95f16a8c1017e309757f23 
> 
> Diff: https://reviews.apache.org/r/45940/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>