You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2017/01/17 04:36:26 UTC
Review Request 55599: CMake: Added `GroupSource` function to automate
IDE source grouping.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55599/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Repository: mesos
Description
-------
CMake allows users to declare groups of source files, which it uses to
(among other things) present source in a directory-like tree of files in
IDEs like XCode and Visual Studio.
Currently this is a manual process: we group the source in each folder
manually and declare it as a source group to CMake.
This commit will introduce a CMake macro that automates this process
away, providing control over many aspects, such as where the group tree
should be rooted, what the root should be named, and so on.
This macro indirectly partially addresses MESOS-3542, which will help us
to separate out Mesos builds into many libraries, rather than one
single, monolithic libmesos.
Diffs
-----
3rdparty/stout/cmake/GroupSource.cmake PRE-CREATION
Diff: https://reviews.apache.org/r/55599/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 55599: CMake: Added `GroupSource` function to
automate IDE source grouping.
Posted by Alex Clemmer <cl...@gmail.com>.
> On Jan. 25, 2017, 12:27 a.m., Joseph Wu wrote:
> > 3rdparty/stout/cmake/GroupSource.cmake, lines 40-41
> > <https://reviews.apache.org/r/55599/diff/1/?file=1606339#file1606339line40>
> >
> > What are the consequences of doing this so inefficiently?
> >
> > Will this make build-file generation slower? Or will it mess up/slow down IDE's?
It will generate the build file slower, but _not_ slow down the IDE, as redundant source groups are not kept.
> On Jan. 25, 2017, 12:27 a.m., Joseph Wu wrote:
> > 3rdparty/stout/cmake/GroupSource.cmake, lines 28-30
> > <https://reviews.apache.org/r/55599/diff/1/?file=1606339#file1606339line28>
> >
> > Another slight tweak, as it is not clearly evident how this shows up.
> >
> > ```
> > For example, if you want to include some master sources in an agent source group, you may provide a ROOT_DIRECTORY `src/master` and a RELATIVE_TO `src/slave`. The master sources would show up as `../master`.
> > ```
> >
> > Note: I'm not sure if this tweak is correct. (Please correct it if so.)
That's not quite right. I fixed the comment, let me know if it's not what you had in mind.
> On Jan. 25, 2017, 12:27 a.m., Joseph Wu wrote:
> > 3rdparty/stout/cmake/GroupSource.cmake, line 82
> > <https://reviews.apache.org/r/55599/diff/1/?file=1606339#file1606339line82>
> >
> > 8 backslashes?
> >
> > So, slashes in `SOURCE_GROUP` are replaced with 4 backslashes here.
> >
> > Later in the `source_group` call, the 4 backslashes translate to 2 backslashes, which still seems to be 1 more backslash than needed.
Ah, sorry, you're actually right. I was somewhat confused by the resolution to MESOS-6736, in which we initially needed 8 `\` characters to generate a string with 4 backslash characters, which evaluated to 2 literal backslash characters, which got passed to the MSVC command line, which in turn the C runtime evaluated to 1 backslash character. Instead of doing that, we just decided to use Unix-style paths which the CRT generally udnerstands (as long as they're not mixed in with Windows-style paths).
Here, though, you are right, it's not necessary. It is sufficient to have 4, which will evaluate to 2 literal backslash characters.
[1] https://issues.apache.org/jira/browse/MESOS-6736
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55599/#review162868
-----------------------------------------------------------
On Jan. 17, 2017, 4:36 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55599/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2017, 4:36 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> CMake allows users to declare groups of source files, which it uses to
> (among other things) present source in a directory-like tree of files in
> IDEs like XCode and Visual Studio.
>
> Currently this is a manual process: we group the source in each folder
> manually and declare it as a source group to CMake.
>
> This commit will introduce a CMake macro that automates this process
> away, providing control over many aspects, such as where the group tree
> should be rooted, what the root should be named, and so on.
>
> This macro indirectly partially addresses MESOS-3542, which will help us
> to separate out Mesos builds into many libraries, rather than one
> single, monolithic libmesos.
>
>
> Diffs
> -----
>
> 3rdparty/stout/cmake/GroupSource.cmake PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55599/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55599: CMake: Added `GroupSource` function to
automate IDE source grouping.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55599/#review162868
-----------------------------------------------------------
3rdparty/stout/cmake/GroupSource.cmake (lines 24 - 26)
<https://reviews.apache.org/r/55599/#comment234255>
I'd like to slightly reword this, as the example actually made question what the argument should be ("slave/" or "Agent Source Files").
```
Specifies group name of this directory tree.
For example, the directory tree `src/slave/` would
fall under the group name "Agent Source Files".
```
3rdparty/stout/cmake/GroupSource.cmake (lines 28 - 30)
<https://reviews.apache.org/r/55599/#comment234256>
Another slight tweak, as it is not clearly evident how this shows up.
```
For example, if you want to include some master sources in an agent source group, you may provide a ROOT_DIRECTORY `src/master` and a RELATIVE_TO `src/slave`. The master sources would show up as `../master`.
```
Note: I'm not sure if this tweak is correct. (Please correct it if so.)
3rdparty/stout/cmake/GroupSource.cmake (lines 40 - 41)
<https://reviews.apache.org/r/55599/#comment234257>
What are the consequences of doing this so inefficiently?
Will this make build-file generation slower? Or will it mess up/slow down IDE's?
3rdparty/stout/cmake/GroupSource.cmake (line 82)
<https://reviews.apache.org/r/55599/#comment234233>
8 backslashes?
So, slashes in `SOURCE_GROUP` are replaced with 4 backslashes here.
Later in the `source_group` call, the 4 backslashes translate to 2 backslashes, which still seems to be 1 more backslash than needed.
- Joseph Wu
On Jan. 16, 2017, 8:36 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55599/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2017, 8:36 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> CMake allows users to declare groups of source files, which it uses to
> (among other things) present source in a directory-like tree of files in
> IDEs like XCode and Visual Studio.
>
> Currently this is a manual process: we group the source in each folder
> manually and declare it as a source group to CMake.
>
> This commit will introduce a CMake macro that automates this process
> away, providing control over many aspects, such as where the group tree
> should be rooted, what the root should be named, and so on.
>
> This macro indirectly partially addresses MESOS-3542, which will help us
> to separate out Mesos builds into many libraries, rather than one
> single, monolithic libmesos.
>
>
> Diffs
> -----
>
> 3rdparty/stout/cmake/GroupSource.cmake PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55599/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55599: CMake: Added `GroupSource` function to
automate IDE source grouping.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55599/#review163184
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On Jan. 26, 2017, 1:12 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55599/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 1:12 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> CMake allows users to declare groups of source files, which it uses to
> (among other things) present source in a directory-like tree of files in
> IDEs like XCode and Visual Studio.
>
> Currently this is a manual process: we group the source in each folder
> manually and declare it as a source group to CMake.
>
> This commit will introduce a CMake macro that automates this process
> away, providing control over many aspects, such as where the group tree
> should be rooted, what the root should be named, and so on.
>
> This macro indirectly partially addresses MESOS-3542, which will help us
> to separate out Mesos builds into many libraries, rather than one
> single, monolithic libmesos.
>
>
> Diffs
> -----
>
> 3rdparty/stout/cmake/GroupSource.cmake PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55599/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 55599: CMake: Added `GroupSource` function to
automate IDE source grouping.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55599/
-----------------------------------------------------------
(Updated Jan. 26, 2017, 9:12 a.m.)
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Changes
-------
Address Joseph's questions.
Repository: mesos
Description
-------
CMake allows users to declare groups of source files, which it uses to
(among other things) present source in a directory-like tree of files in
IDEs like XCode and Visual Studio.
Currently this is a manual process: we group the source in each folder
manually and declare it as a source group to CMake.
This commit will introduce a CMake macro that automates this process
away, providing control over many aspects, such as where the group tree
should be rooted, what the root should be named, and so on.
This macro indirectly partially addresses MESOS-3542, which will help us
to separate out Mesos builds into many libraries, rather than one
single, monolithic libmesos.
Diffs (updated)
-----
3rdparty/stout/cmake/GroupSource.cmake PRE-CREATION
Diff: https://reviews.apache.org/r/55599/diff/
Testing
-------
Thanks,
Alex Clemmer