mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 55599: CMake: Added `GroupSource` function to automate IDE source grouping.
Date Thu, 26 Jan 2017 09:10:22 GMT


> 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
> 
>


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