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 2018/03/09 22:39:14 UTC
Review Request 66010: Windows: Switched to default CRT linkage.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66010/
-----------------------------------------------------------
Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
Repository: mesos
Description
-------
We previously attempted to manually override the CRT to be static
everywhere. Not only did this emit warnings, it was also error-prone
and unnecessary. We can, and should, just use the defaults, which is
`/MDd` in debug mode (multi-threaded, dynamic, debug linkage). Linking
to the CRT dynamically results in smaller libraries and executables,
reduces linking time, and avoids bugs when sharing allocated memory
across modules.
Diffs
-----
cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54
Diff: https://reviews.apache.org/r/66010/diff/1/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 66010: Windows: Switched to default CRT linkage.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66010/#review199031
-----------------------------------------------------------
Ship it!
Ship It!
- John Kordich
On March 9, 2018, 10:39 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66010/
> -----------------------------------------------------------
>
> (Updated March 9, 2018, 10:39 p.m.)
>
>
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8659
> https://issues.apache.org/jira/browse/MESOS-8659
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We previously attempted to manually override the CRT to be static
> everywhere. Not only did this emit warnings, it was also error-prone
> and unnecessary. We can, and should, just use the defaults, which is
> `/MDd` in debug mode (multi-threaded, dynamic, debug linkage). Linking
> to the CRT dynamically results in smaller libraries and executables,
> reduces linking time, and avoids bugs when sharing allocated memory
> across modules.
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54
>
>
> Diff: https://reviews.apache.org/r/66010/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66010: Windows: Switched to default CRT linkage.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On March 16, 2018, 3:08 p.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 326-330 (original), 326-330 (patched)
> > <https://reviews.apache.org/r/66010/diff/2/?file=1975943#file1975943line326>
> >
> > Looks like this variable can go away too.
Indeed! Thank you :) I can do that before I commit.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66010/#review199355
-----------------------------------------------------------
On March 13, 2018, 1:45 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66010/
> -----------------------------------------------------------
>
> (Updated March 13, 2018, 1:45 p.m.)
>
>
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8659
> https://issues.apache.org/jira/browse/MESOS-8659
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We previously attempted to manually override the CRT to be static
> everywhere. Not only did this emit warnings, it was also error-prone
> and unnecessary. We can, and should, just use the defaults, which is
> `/MDd` in debug mode (multi-threaded, dynamic, debug linkage). Linking
> to the CRT dynamically results in smaller libraries and executables,
> reduces linking time, and avoids bugs when sharing allocated memory
> across modules.
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54
>
>
> Diff: https://reviews.apache.org/r/66010/diff/2/
>
>
> Testing
> -------
>
> NOTE: I checked with Alex Ionescu and since we are deploying on the same OS versions as we're targetting, we don't need to worry about redistributing the CRT.
>
> If/when we start deploying on older OS versions, we can simply add instructions on installing the correct CRT redistributable (or make it part of the eventual installer).
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66010: Windows: Switched to default CRT linkage.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66010/#review199355
-----------------------------------------------------------
Fix it, then Ship it!
cmake/CompilationConfigure.cmake
Lines 326-330 (original), 326-330 (patched)
<https://reviews.apache.org/r/66010/#comment279664>
Looks like this variable can go away too.
- Joseph Wu
On March 13, 2018, 1:45 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66010/
> -----------------------------------------------------------
>
> (Updated March 13, 2018, 1:45 p.m.)
>
>
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
>
>
> Bugs: MESOS-8659
> https://issues.apache.org/jira/browse/MESOS-8659
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We previously attempted to manually override the CRT to be static
> everywhere. Not only did this emit warnings, it was also error-prone
> and unnecessary. We can, and should, just use the defaults, which is
> `/MDd` in debug mode (multi-threaded, dynamic, debug linkage). Linking
> to the CRT dynamically results in smaller libraries and executables,
> reduces linking time, and avoids bugs when sharing allocated memory
> across modules.
>
>
> Diffs
> -----
>
> cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54
>
>
> Diff: https://reviews.apache.org/r/66010/diff/2/
>
>
> Testing
> -------
>
> NOTE: I checked with Alex Ionescu and since we are deploying on the same OS versions as we're targetting, we don't need to worry about redistributing the CRT.
>
> If/when we start deploying on older OS versions, we can simply add instructions on installing the correct CRT redistributable (or make it part of the eventual installer).
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 66010: Windows: Switched to default CRT linkage.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66010/
-----------------------------------------------------------
(Updated March 19, 2018, 12:12 p.m.)
Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
Changes
-------
Removed superfluous variable.
Bugs: MESOS-8659
https://issues.apache.org/jira/browse/MESOS-8659
Repository: mesos
Description
-------
We previously attempted to manually override the CRT to be static
everywhere. Not only did this emit warnings, it was also error-prone
and unnecessary. We can, and should, just use the defaults, which is
`/MDd` in debug mode (multi-threaded, dynamic, debug linkage). Linking
to the CRT dynamically results in smaller libraries and executables,
reduces linking time, and avoids bugs when sharing allocated memory
across modules.
Diffs (updated)
-----
cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54
Diff: https://reviews.apache.org/r/66010/diff/3/
Changes: https://reviews.apache.org/r/66010/diff/2-3/
Testing
-------
NOTE: I checked with Alex Ionescu and since we are deploying on the same OS versions as we're targetting, we don't need to worry about redistributing the CRT.
If/when we start deploying on older OS versions, we can simply add instructions on installing the correct CRT redistributable (or make it part of the eventual installer).
Thanks,
Andrew Schwartzmeyer
Re: Review Request 66010: Windows: Switched to default CRT linkage.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66010/
-----------------------------------------------------------
(Updated March 13, 2018, 1:45 p.m.)
Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
Bugs: MESOS-8659
https://issues.apache.org/jira/browse/MESOS-8659
Repository: mesos
Description
-------
We previously attempted to manually override the CRT to be static
everywhere. Not only did this emit warnings, it was also error-prone
and unnecessary. We can, and should, just use the defaults, which is
`/MDd` in debug mode (multi-threaded, dynamic, debug linkage). Linking
to the CRT dynamically results in smaller libraries and executables,
reduces linking time, and avoids bugs when sharing allocated memory
across modules.
Diffs
-----
cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54
Diff: https://reviews.apache.org/r/66010/diff/1/
Testing (updated)
-------
NOTE: I checked with Alex Ionescu and since we are deploying on the same OS versions as we're targetting, we don't need to worry about redistributing the CRT.
If/when we start deploying on older OS versions, we can simply add instructions on installing the correct CRT redistributable (or make it part of the eventual installer).
Thanks,
Andrew Schwartzmeyer