Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Cygwin CMake Build #2101

Closed
wants to merge 2 commits into from
Closed

Conversation

MaEtUgR
Copy link
Contributor

@MaEtUgR MaEtUgR commented Feb 4, 2019

Why?
Based on the findings I wrote here I'm fixing the global CMakeLists file such that building under Cygwin also works out of the box e.g. when following the readme and including the repo in your CMake project.

On Cygwin specifying set(CMAKE_CXX_EXTENSIONS OFF) breaks the build exactly like described in https://stackoverflow.com/q/18784112/6326048

  • add_definitions(-std=c++11) also breaks it
  • Removing the whole block of compile std defines builds fine

@peterjc123 Is set(CMAKE_CXX_EXTENSIONS OFF) really necessary for you? If yes I'll create a pr to only exclude the line for Cygwin to makes sure it builds out of the box on that platform as well.

What?

  • First commit To detect Cygwin I had to move the project initialization to the beginning, see https://stackoverflow.com/a/35914521/6326048
  • Second commit I'm not disabling C++ extensions when building the repository using the global CMake file in a Cygwin environment.

Really?
Here's the output after the following commands in the freshly cloned repo wit the latest Cygwin environment:

mkdir build
cd build
cmake ..
make

Before change:
gtest-fail
After change:
gtest-success

Additional info
Building gtest without any C++ standard definitions works fine on Cygwin, probably the defaults are just right. That's also why diectly building it using the CMakeLists.txt files in the googletest or googlemock folder is fine. I'm fixing the global one because I intend to use it and it's also frustrating to new users to first have a failing build.

such that necessary cmake variables to detect Cygwin are defined
when setting the C++ standard and we can distinguish.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@MaEtUgR
Copy link
Contributor Author

MaEtUgR commented Feb 4, 2019

I signed it! 😃

@googlebot
Copy link

CLAs look good, thanks!

gennadiycivil added a commit that referenced this pull request Feb 8, 2019
@gennadiycivil
Copy link
Contributor

merged now

gennadiycivil added a commit that referenced this pull request Feb 12, 2019
gennadiycivil added a commit that referenced this pull request Feb 12, 2019
gennadiycivil added a commit that referenced this pull request Feb 12, 2019
gennadiycivil added a commit that referenced this pull request Feb 12, 2019
gennadiycivil added a commit that referenced this pull request Feb 12, 2019
@MaEtUgR
Copy link
Contributor Author

MaEtUgR commented Feb 19, 2019

@gennadiycivil I just tested and it works perfect thanks! I could probably contribute CI on Cygwin using appveyor if there's any interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants