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

Add cppstd as a subsetting of compiler #4917

Merged
merged 73 commits into from May 3, 2019

Conversation

Projects
None yet
4 participants
@jgsogo
Copy link
Member

commented Apr 5, 2019

Changelog: Feature: Add compiler.cppstd setting (mark cppstd as deprecated)
Docs: conan-io/docs#1266

This PR includes changes from PR #4986 and #4942 (really short ones)


Some hints about the new/old behavior:

  • It keeps previous behavior: all use cases (tests) should be summarized here #4986,
  • The default value (or None) for compiler.cppstd generates the same ID as it was generating for cppstd.
  • If the user specifies cppstd and compiler.cppstd, Conan will raise. Yes, even if those have the same values.
  • Package ID for non-default values generates different ID for cppstd and compiler.cppstd
  • If no value is provided to compiler.cppstd then the default value will be assigned to it.

Summarizing:

package_id("cppstd=default") == package_id("cppstd=None")
package_id("cppstd=default") == package_id("compiler.cppstd=default")
package_id("compiler.cppstd=default") == package_id("compiler.cppstd=None")
package_id("cppstd=20") != package_id("compiler.cppstd=20")

closes #4873

@tags: slow

@ghost ghost assigned jgsogo Apr 5, 2019

@ghost ghost added the stage: review label Apr 5, 2019

@jgsogo jgsogo added this to the 1.15 milestone Apr 5, 2019

@jgsogo jgsogo force-pushed the jgsogo:feat/4873-cppstd-subsetting branch from e49e8db to fc87bbb Apr 5, 2019

@jgsogo jgsogo added the type: feature label Apr 6, 2019

@jgsogo jgsogo marked this pull request as ready for review Apr 10, 2019

@jgsogo jgsogo requested a review from lasote Apr 10, 2019

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Relevant changes since last reviews:

  • Use compiler.cppstd in build_helpers, until now I wasn't taking into account compiler.cppstd in the build helpers 😓. I've written a helper function cppstd_from_settings, but not sure if conans/client/build/cppstd_flags.py is the place to define it.
  • Pure C recipes will need to remove compiler.cppstd (as they were already removing compiler.libcxx) or Conan will inject the compiler flag for the C++ standard and some compilers will raise an error
    • So the autogenerated conanfile.py for testing has a different checksum.
  • Docs already available

@jgsogo jgsogo requested review from memsharded and lasote Apr 30, 2019

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

What happens to existing pure C libraries that are already removing compiler.libcxx, but will not be removing the default added compiler.cppstd? Will the ID be affected?

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

What happens to existing pure C libraries that are already removing compiler.libcxx, but will not be removing the default added compiler.cppstd? Will the ID be affected?

The problem is not the ID (it won't be affected because the compiler.cppstd=<default> is not used to compute the ID), the problem is that those libraries could fail if the compiler raises for the new flag...

image

...and I'm not sure if we can handle this use case, as we can't know that those libraries are pure C. Probably this is a breaking change ⚠️ ⚠️ ⚠️

@memsharded
Copy link
Contributor

left a comment

I think this is a problem, it is a breaking change, we can't do this. In summary:

  • Automatically setting a compiler.cppstd value to the default and,
  • Automatically using compiler.cppstd in build helpers =>
  • Will produce an error in C packages, that won't allow that compiler flag.

I think the only way to introduce this is to not set the default compiler.cppstd

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

Thinking about this breaking change a little bit deeper:

  • This flag is being introduced in the following elements:
    • AutoToolsBuildEnvironment: this new flag goes to CXXFLAGS
    • Meson: goes to ... -D cpp_std="c++14". And that option is different from c_std (docs).
    • CMake: populates:
      set(CONAN_CMAKE_CXX_STANDARD 14)
      set(CONAN_CMAKE_CXX_EXTENSIONS ON/OFF)
      set(CONAN_STD_CXX_FLAG c++14)
      ...
      set(CMAKE_CXX_FLAGS "${CONAN_STD_CXX_FLAG} ${CMAKE_CXX_FLAGS}")  # CMake <= 3.1
      set(CMAKE_CXX_STANDARD ${CONAN_CMAKE_CXX_STANDARD})
      set(CMAKE_CXX_EXTENSIONS ${CONAN_CMAKE_CXX_EXTENSIONS})
      
    • VisualStudioBuildEnvironment: goes to dict entry "CL": "... /std:c++14 ..." which is then used by MSBuild and VirtualBuildEnvGenerator.
    • CompilerArgsGenerator: it goes directly to the list of flags

So, I think that the problem could be with CompilerArgsGenerator (it was the test that failed and I commented before), but all the other ones should avoid this flag when compiling a C library.

@jgsogo jgsogo requested a review from memsharded May 3, 2019

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

I need to modify docs according to the new behavior

@lasote
Copy link
Contributor

left a comment

Looking good!

@@ -288,3 +288,75 @@
settings_1_14_2 = settings_1_14_1
settings_1_14_3 = settings_1_14_2
settings_1_14_4 = settings_1_14_3

settings_1_15_0 = """

This comment has been minimized.

Copy link
@lasote

lasote May 3, 2019

Contributor

I think this might be better to be adjusted when the release is merged to develop probably. In that point, we have "freeze" the settings for the version, not before. Why did you add it?

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Docs ready

@lasote

lasote approved these changes May 3, 2019

@lasote lasote merged commit a050ed9 into conan-io:develop May 3, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label May 3, 2019

@jgsogo jgsogo deleted the jgsogo:feat/4873-cppstd-subsetting branch May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.