Skip to content

Comments

cmake: Don't forcibly export C++ version#52

Merged
ddemidov merged 3 commits intoddemidov:masterfrom
alexdewar:cmake_fix
Apr 26, 2019
Merged

cmake: Don't forcibly export C++ version#52
ddemidov merged 3 commits intoddemidov:masterfrom
alexdewar:cmake_fix

Conversation

@alexdewar
Copy link
Contributor

This PR fixes issue #51, if the user has cmake v3.1 or newer, which is almost certainly most users.

For older versions of cmake there doesn't seem to be a nice way to check if the user has already specified a C++ version, so I have preserved the old behaviour where the C++ version is also set for any parent projects.

@ddemidov
Copy link
Owner

Travis reports a compile error:
https://travis-ci.org/ddemidov/ev3dev-lang-cpp/builds/524842540

@ddemidov
Copy link
Owner

Another option would be to set public target_compile_features on the library (example).

@alexdewar
Copy link
Contributor Author

Rats. It looks like now the C++ version isn't being set to 11 when it should be.

The problem with using target_compile_features is that I'd like to be able to just use C++14 without having to specify which features of it I need.

I've tried tweaking it to see if this fixes things.

@alexdewar
Copy link
Contributor Author

Ok, got it on the third attempt! It seems that Travis wasn't correctly setting the C++ version for gcc -- not sure why (it's a new version of cmake but an old version of gcc so maybe that's the problem).

So now we set the C++ version in two ways, which is a bit gross, but at least works and solves the original problem.

CMakeLists.txt Outdated
# CMake on Travis seems not to be setting the C++ version correctly, even
# when specified as below (maybe because it's an old version of gcc?). So
# let's set this as a private compile option for a belt-and-braces approach.
target_compile_options(ev3dev PRIVATE -std=c++0x)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure it is safe to compile the library with c++11 and then link it with the code that was compiled with c++14? Are there no ABI incompatibilities?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there should be (C++ is supposed to be backwards compatible with this kind of thing...) and it does work for me.

Also it's quite a common thing to do: lots of C++ libs are compiled for C++11 but you can still link against them with programs using newer versions of C++.

@ddemidov
Copy link
Owner

I did a quick test in 11c2063, and it seems that just defining CMAKE_CXX_STANDARD earlier in CMakeLists.txt is enough:

https://travis-ci.org/ddemidov/ev3dev-lang-cpp/builds/524885989

So may be there is no need to test for CMAKE_VERSION?

@ddemidov
Copy link
Owner

Sorry, forgot to also remove -std=c++0x. Checking now:
https://travis-ci.org/ddemidov/ev3dev-lang-cpp/builds/524890788

@alexdewar
Copy link
Contributor Author

That would be cleaner, but I believe CMAKE_CXX_STANDARD was only introduced in cmake v3.1. Could always just bump the cmake_minimum_required to 3.1 though, if you're sure that no one will be using an older version.

@ddemidov
Copy link
Owner

Looks like Debian stretch (the current distribution used by ev3dev) has cmake 3.7:

https://packages.debian.org/stretch/devel/cmake

so bumping the required version to 3.1 should be ok.

@alexdewar
Copy link
Contributor Author

That all looks good to me. I've tested it with my project and now it works fine. Cheers :-)

@ddemidov
Copy link
Owner

Can you update this PR so I can merge to master?

@alexdewar
Copy link
Contributor Author

Done :-)

@ddemidov ddemidov merged commit 28e3d31 into ddemidov:master Apr 26, 2019
@ddemidov
Copy link
Owner

Thank you!

@alexdewar
Copy link
Contributor Author

Thanks for being so responsive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants