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

Removed activate_cxx11 macro. #73

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Removed activate_cxx11 macro. #73

wants to merge 3 commits into from

Conversation

skasperski
Copy link

It also removes the adding of -std=cxx00 to the pkg-config file. This is because in CMake the CXX_STANDARD property is interpreted as a minimum requirement. This cannot be translated to a linker-flag, which will set the standard to exactly what was given.

Copy link
Member

@doudou doudou left a comment

Choose a reason for hiding this comment

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

Executive Summary:

  1. add a warning message to activate_cxx11, but leave the macro as-is
  2. either remove the second change completely, or put it in a different pull request (see below for the "removing" rationale)

Removing something is not deprecating it. If some packages rely on this macro, and for them it works, then removing it might (will ?) break them. Add a warning message that the macro should not be used instead, but leave the rest as-is.

You create a pull request to "remove a macro" and then also change the .pc files generated, in a codepath that is not directly related to it. This second change will most probably (and obviously) have rather extensive consequences. It cannot be done as-is.

First of all, I don't interpret the documentation of the CXX_STANDARD property as you do. CMake's documentation explicitly says that setting CXX_STANDARD will lead to having the -std flag set to the given standard if it available. What's really really bad is that it might downgrade (I'm going to go for _REQUIRED for now on). I definitely don't interpret the documentation in terms that it might upgrade.

https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html

Even if you were right on that interpretation, if you care about the exact flag being used, try to identify what CMake will use to compile the package and propagate that flag. The current situation is second-best. Not putting anything is plain wrong, and will definitely break existing builds (it just blew my continuous integration build).

@skasperski
Copy link
Author

1. add a warning message to activate_cxx11, but leave the macro as-is

2. either remove the second change completely, or put it in a different pull request (see below for the "removing" rationale)

If I do 1. (which does nothing) and 2. as well, this PR is obsolete and can be closed.

Which leaves the question how the current situation is solved instead. As it is now, a CMake package that correctly requests C++11 by setting CXX_STANDARD, effectively leads to having "-std=cxx11" in it's .pc files, thus breaking the build for downstream packages that require a higher standard.

@planthaber
Copy link
Member

If some packages rely on this macro, and for them it works, then removing it might (will ?) break them.

Ubuntu 16.04 is officially unsupported and gcc versions in newer ubuntus are using cxx14 as default. I don't see ho removing the downgrade do cxx11 would cause failing builds. Or do I miss something here?

@doudou
Copy link
Member

doudou commented Sep 7, 2021

Ubuntu 16.04 is officially unsupported and gcc versions in newer ubuntus are using cxx14 as default. I don't see ho removing the downgrade do cxx11 would cause failing builds. Or do I miss something here?

I costs zero to keep the macro, and from our side it ensures that we actually do not discover after the fact we have missed something. The default is irrelevant, if a package is currently being compiled with -std=c++11 there is zero reason to change that on our side. The best we can do is warn people that they should really consider doing things differently.

@doudou
Copy link
Member

doudou commented Sep 7, 2021

Which leaves the question how the current situation is solved instead. As it is now, a CMake package that correctly requests C++11 by setting CXX_STANDARD, effectively leads to having "-std=cxx11" in it's .pc files, thus breaking the build for downstream packages that require a higher standard.

After the change, how will you fix downstream packages that were essentially compiled with, say, C++17 because of such a flag and won't be anymore ?

The best way for a CMake package to deal with this would be to parse the Cflags from the pkg-config when it gets loaded, detect the -std flag and propagate the setting through CXX_STANDARD (if that property was not already set).

Whatever you do, please create package(s) for each case you want to fix within the build_tests and add them to https://github.com/rock-core/rock.build-tests-buildconf. See for instance https://github.com/rock-core/build_tests-cmake-var_ROCK_PUBLIC_CXX_STANDARD.

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.

3 participants