You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2017/05/10 20:32:05 UTC
Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/
-----------------------------------------------------------
Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
Bugs: MESOS-7496
https://issues.apache.org/jira/browse/MESOS-7496
Repository: mesos
Description
-------
A previous commit attempted to enable this option, but it was not
actually being used by the linker. This patch correctly adds the option
to each linker flag variable for shared libraries, modules, and
executables, and only for the debug configuration. This results in much
faster link times.
Also, the `/zc:inline` option was removed, as it is only relevant in
optimized builds (i.e. release configurations).
Diffs
-----
cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
Diff: https://reviews.apache.org/r/59155/diff/1/
Testing
-------
Built and tested on Windows with VS Code.
NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On May 10, 2017, 8:37 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 239 (patched)
> > <https://reviews.apache.org/r/59155/diff/1/?file=1714219#file1714219line244>
> >
> > What about `STATIC`? All libraries are built with static linkage on Windows (at the moment).
>
> Andrew Schwartzmeyer wrote:
> Oh, uh, heh. This `foreach` loop came straight from [CMake](https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/Platform/Windows-MSVC.cmake#L163). Checking if `CMAKE_STATIC_LINKER_FLAGS_DEBUG` is the right one to add.
Yup, adding.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/#review174547
-----------------------------------------------------------
On May 10, 2017, 8:32 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59155/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 8:32 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-7496
> https://issues.apache.org/jira/browse/MESOS-7496
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A previous commit attempted to enable this option, but it was not
> actually being used by the linker. This patch correctly adds the option
> to each linker flag variable for shared libraries, modules, and
> executables, and only for the debug configuration. This results in much
> faster link times.
>
> Also, the `/zc:inline` option was removed, as it is only relevant in
> optimized builds (i.e. release configurations).
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
>
>
> Diff: https://reviews.apache.org/r/59155/diff/1/
>
>
> Testing
> -------
>
> Built and tested on Windows with VS Code.
>
> NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On May 10, 2017, 8:37 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 239 (patched)
> > <https://reviews.apache.org/r/59155/diff/1/?file=1714219#file1714219line244>
> >
> > What about `STATIC`? All libraries are built with static linkage on Windows (at the moment).
Oh, uh, heh. This `foreach` loop came straight from [CMake](https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/Platform/Windows-MSVC.cmake#L163). Checking if `CMAKE_STATIC_LINKER_FLAGS_DEBUG` is the right one to add.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/#review174547
-----------------------------------------------------------
On May 10, 2017, 8:32 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59155/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 8:32 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-7496
> https://issues.apache.org/jira/browse/MESOS-7496
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A previous commit attempted to enable this option, but it was not
> actually being used by the linker. This patch correctly adds the option
> to each linker flag variable for shared libraries, modules, and
> executables, and only for the debug configuration. This results in much
> faster link times.
>
> Also, the `/zc:inline` option was removed, as it is only relevant in
> optimized builds (i.e. release configurations).
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
>
>
> Diff: https://reviews.apache.org/r/59155/diff/1/
>
>
> Testing
> -------
>
> Built and tested on Windows with VS Code.
>
> NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/#review174547
-----------------------------------------------------------
cmake/CompilationConfigure.cmake
Lines 239 (patched)
<https://reviews.apache.org/r/59155/#comment247703>
What about `STATIC`? All libraries are built with static linkage on Windows (at the moment).
- Joseph Wu
On May 10, 2017, 1:32 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59155/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 1:32 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-7496
> https://issues.apache.org/jira/browse/MESOS-7496
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A previous commit attempted to enable this option, but it was not
> actually being used by the linker. This patch correctly adds the option
> to each linker flag variable for shared libraries, modules, and
> executables, and only for the debug configuration. This results in much
> faster link times.
>
> Also, the `/zc:inline` option was removed, as it is only relevant in
> optimized builds (i.e. release configurations).
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
>
>
> Diff: https://reviews.apache.org/r/59155/diff/1/
>
>
> Testing
> -------
>
> Built and tested on Windows with VS Code.
>
> NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/#review174555
-----------------------------------------------------------
Ship it!
Ship It!
- Jeff Coffler
On May 10, 2017, 8:59 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59155/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 8:59 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-7496
> https://issues.apache.org/jira/browse/MESOS-7496
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A previous commit attempted to enable this option, but it was not
> actually being used by the linker. This patch correctly adds the option
> to each linker flag variable for shared libraries, modules, and
> executables, and only for the debug configuration. This results in much
> faster link times.
>
> Also, the `/zc:inline` option was removed, as it is only relevant in
> optimized builds (i.e. release configurations).
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
>
>
> Diff: https://reviews.apache.org/r/59155/diff/2/
>
>
> Testing
> -------
>
> Built and tested on Windows with VS Code.
>
> NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
Posted by Li Li <li...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/#review174601
-----------------------------------------------------------
Ship it!
Ship It!
- Li Li
On May 10, 2017, 8:59 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59155/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 8:59 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-7496
> https://issues.apache.org/jira/browse/MESOS-7496
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A previous commit attempted to enable this option, but it was not
> actually being used by the linker. This patch correctly adds the option
> to each linker flag variable for shared libraries, modules, and
> executables, and only for the debug configuration. This results in much
> faster link times.
>
> Also, the `/zc:inline` option was removed, as it is only relevant in
> optimized builds (i.e. release configurations).
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
>
>
> Diff: https://reviews.apache.org/r/59155/diff/2/
>
>
> Testing
> -------
>
> Built and tested on Windows with VS Code.
>
> NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/#review174985
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On May 10, 2017, 1:59 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59155/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 1:59 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-7496
> https://issues.apache.org/jira/browse/MESOS-7496
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A previous commit attempted to enable this option, but it was not
> actually being used by the linker. This patch correctly adds the option
> to each linker flag variable for shared libraries, modules, and
> executables, and only for the debug configuration. This results in much
> faster link times.
>
> Also, the `/zc:inline` option was removed, as it is only relevant in
> optimized builds (i.e. release configurations).
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
>
>
> Diff: https://reviews.apache.org/r/59155/diff/2/
>
>
> Testing
> -------
>
> Built and tested on Windows with VS Code.
>
> NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
Posted by Li Li <li...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/#review174602
-----------------------------------------------------------
Ship it!
Ship It!
- Li Li
On May 10, 2017, 8:59 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59155/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 8:59 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-7496
> https://issues.apache.org/jira/browse/MESOS-7496
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A previous commit attempted to enable this option, but it was not
> actually being used by the linker. This patch correctly adds the option
> to each linker flag variable for shared libraries, modules, and
> executables, and only for the debug configuration. This results in much
> faster link times.
>
> Also, the `/zc:inline` option was removed, as it is only relevant in
> optimized builds (i.e. release configurations).
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
>
>
> Diff: https://reviews.apache.org/r/59155/diff/2/
>
>
> Testing
> -------
>
> Built and tested on Windows with VS Code.
>
> NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On May 10, 2017, 9:07 p.m., Jeff Coffler wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 240 (patched)
> > <https://reviews.apache.org/r/59155/diff/2/?file=1714308#file1714308line245>
> >
> > I'm not thrilled that we need to do a "hand patch" to debug using VS Code. I think the vast majority of Windows DEVS will hit this.
> >
> > But it is what it is. I'm not entirely sure how this didn't make it when I added it, I could have sworn I looked at the command lines. But I guess not. Or I did but a last-minute change broke it.
Yeah, no worries. My opinion is that the faster build is worth more than having to hand-patch the extension for now, especially since it's a known issue that will be fixed with the next extension release (I confirmed this with an author of the extension).
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/#review174554
-----------------------------------------------------------
On May 10, 2017, 8:59 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59155/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 8:59 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-7496
> https://issues.apache.org/jira/browse/MESOS-7496
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A previous commit attempted to enable this option, but it was not
> actually being used by the linker. This patch correctly adds the option
> to each linker flag variable for shared libraries, modules, and
> executables, and only for the debug configuration. This results in much
> faster link times.
>
> Also, the `/zc:inline` option was removed, as it is only relevant in
> optimized builds (i.e. release configurations).
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
>
>
> Diff: https://reviews.apache.org/r/59155/diff/2/
>
>
> Testing
> -------
>
> Built and tested on Windows with VS Code.
>
> NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/#review174554
-----------------------------------------------------------
cmake/CompilationConfigure.cmake
Lines 240 (patched)
<https://reviews.apache.org/r/59155/#comment247706>
I'm not thrilled that we need to do a "hand patch" to debug using VS Code. I think the vast majority of Windows DEVS will hit this.
But it is what it is. I'm not entirely sure how this didn't make it when I added it, I could have sworn I looked at the command lines. But I guess not. Or I did but a last-minute change broke it.
- Jeff Coffler
On May 10, 2017, 8:59 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59155/
> -----------------------------------------------------------
>
> (Updated May 10, 2017, 8:59 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
>
>
> Bugs: MESOS-7496
> https://issues.apache.org/jira/browse/MESOS-7496
>
>
> Repository: mesos
>
>
> Description
> -------
>
> A previous commit attempted to enable this option, but it was not
> actually being used by the linker. This patch correctly adds the option
> to each linker flag variable for shared libraries, modules, and
> executables, and only for the debug configuration. This results in much
> faster link times.
>
> Also, the `/zc:inline` option was removed, as it is only relevant in
> optimized builds (i.e. release configurations).
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
>
>
> Diff: https://reviews.apache.org/r/59155/diff/2/
>
>
> Testing
> -------
>
> Built and tested on Windows with VS Code.
>
> NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 59155: CMake: Enable `/debug:fastlink` on Windows.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59155/
-----------------------------------------------------------
(Updated May 10, 2017, 8:59 p.m.)
Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
Bugs: MESOS-7496
https://issues.apache.org/jira/browse/MESOS-7496
Repository: mesos
Description (updated)
-------
A previous commit attempted to enable this option, but it was not
actually being used by the linker. This patch correctly adds the option
to each linker flag variable for shared libraries, modules, and
executables, and only for the debug configuration. This results in much
faster link times.
Also, the `/zc:inline` option was removed, as it is only relevant in
optimized builds (i.e. release configurations).
Diffs (updated)
-----
cmake/CompilationConfigure.cmake 7b2669f0c54abf9b5e0fd60f8af01e97e1f0f86a
Diff: https://reviews.apache.org/r/59155/diff/2/
Changes: https://reviews.apache.org/r/59155/diff/1-2/
Testing
-------
Built and tested on Windows with VS Code.
NOTE: With `/debug:fastlink` properly enabled, the current VS Code C++ debugger does not work without a manual update. See [this thread](https://github.com/Microsoft/vscode-cpptools/issues/695#issuecomment-299554289) for the work-around.
Thanks,
Andrew Schwartzmeyer