Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Add gtest_force_shared_crt for MSVC #284

Conversation

meastp
Copy link
Contributor

@meastp meastp commented Feb 19, 2019

No description provided.

@g-easy
Copy link
Contributor

g-easy commented Feb 20, 2019

Please describe why this is being done.

@meastp
Copy link
Contributor Author

meastp commented Feb 20, 2019

This is being done simply because the build (or, to be more specifik, the linking) on MSVC fails when abseil and opencensus-cpp compile with /MD, and googletest compiles with /MT.

  # force this option to ON so that Google Test will use /MD instead of /MT
# /MD is now the default for Visual Studio, so it should be our default, too

@g-easy
Copy link
Contributor

g-easy commented Feb 20, 2019

Makes sense. Thanks a lot for the followup. Please run tools/format.sh

@meastp
Copy link
Contributor Author

meastp commented Feb 20, 2019

I'm on Windows, so I can not run tools/format.sh directly, but by reading the script I think only the following applies to my changes (I'm not familiar with the buildify command, so I don't know if it applies):

No CRLF line endings.

sed -i 's/\r$//' $($FIND -type f -print)

No trailing spaces.

sed -i 's/ +$//' $($FIND -type f -print)

If this is correct I'll fix that. Please confirm that my understanding is correct... :)

@g-easy
Copy link
Contributor

g-easy commented Feb 21, 2019

Those definitely apply, but cmake-format might be in play for this PR as well.

@g-easy
Copy link
Contributor

g-easy commented Feb 21, 2019

It's not just whitespace. I did the format on my end but I don't think I can push it to this PR:

git checkout -b meastp-meastp_msvc_add_gtest_force_shared_crt master
git pull https://github.com/meastp/opencensus-cpp.git meastp_msvc_add_gtest_force_shared_crt
tools/format.sh
git commit -a -m format
git push g-easy meastp-meastp_msvc_add_gtest_force_shared_crt

Please apply this commit to the PR: g-easy@17cc862

@g-easy
Copy link
Contributor

g-easy commented Feb 22, 2019

If you prefer, I can try to do it in the online editor. :)

@g-easy g-easy self-requested a review February 22, 2019 03:16
@meastp
Copy link
Contributor Author

meastp commented Feb 22, 2019

Thanks, I managed to merge your commit online by creating a pull request with compare from your repository (using your meastp_msvc_add_gtest_force_shared_crt-branch, obviously) to my repository :)

@g-easy g-easy merged commit 03dff03 into census-instrumentation:master Feb 22, 2019
@meastp meastp mentioned this pull request Feb 25, 2019
16 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants