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

Configuration not propagated to compiler defines #581

Closed
AnthonyVH opened this issue Jun 4, 2024 · 12 comments
Closed

Configuration not propagated to compiler defines #581

AnthonyVH opened this issue Jun 4, 2024 · 12 comments

Comments

@AnthonyVH
Copy link

AnthonyVH commented Jun 4, 2024

I'm trying to disable the std::format support when compiling with Clang 17 in C++23 mode (i.e. std::format is definitely supported). I have been trying multiple things for hours now, and can't find any way to make this work.

Here's (part of) my conanfile.py:

  def configure(self):
        self.options["mp-units"].std_format = False

   def requirements(self):
        self.requires("mp-units/2.2.0@mpusz/testing")

And part of the CMakeLists.txt file:

find_package(mp-units REQUIRED)

target_link_libraries(${PROJECT_NAME}
  PUBLIC
    mp-units::mp-units
)

No matter what I try, MP_UNITS_API_STD_FORMAT doesn't get defined when compiling. This then causes MP_UNITS_STD_FMT to be set to std, instead of fmt which is what I want. I tested this by adding some extra preprocessor checks/prints to src/core/include/mp-units/compat_macros.h

I tried setting MP_UNITS_API_STD_FORMAT via my project's CMakeLists.txt file to "FALSE", or OFF, but neither does the trick. I'm very confused, because it seems the set_feature_flag() function in src/core/CMakeLists.txt should define MP_UNITS_API_STD_FORMAT. And as far as I understand, I shouldn't need to do this anyway, because mp-units' conanfile.py sets these variables in its generate() function.

Although I'm a bit confused that the manual states that MP_UNITS_API_STD_FORMAT should be set to "AUTO"/"ON"/"OFF", but then the set_feature_flag() checks that it's one of "TRUE" or "FALSE". But again, this shouldn't matter when using conan, because it will set it to either "FALSE" or "TRUE".

I hope I'm just making a silly mistake, because I have no clue what I'm doing wrong and what else to try.

@mpusz
Copy link
Owner

mpusz commented Jun 4, 2024

Hi @AnthonyVH, I am not sure if the syntax self.options["mp-units"] is still valid in Conan 2.0? Maybe you should do:

self.dependencies["mp-units"].options.std_format = False

?

Please also note that as described here, setting std_format to False switches to fmtlib usage. Text formatting is still there. If your intent is to get rid of all the text formatting because, for example, you work with bare metal hardware, then freestanding option should be used.

BTW, Clang 17 does not provide __cpp_lib_format as it is not fully supported by this compiler.

@mpusz
Copy link
Owner

mpusz commented Jun 4, 2024

the manual states that MP_UNITS_API_STD_FORMAT should be set to "AUTO"/"ON"/"OFF", but then the set_feature_flag() checks that it's one of "TRUE" or "FALSE"

You are right. This is a bug on my side. I have to think about how to fix it.

@AnthonyVH
Copy link
Author

AnthonyVH commented Jun 4, 2024

I'm not trying to get rid of text formatting. We're using fmt in our project, so that's what I want mp-units to use as well.

self.dependencies["mp-units"].options.std_format = False

The conan manual states that self.dependencies is not accessible from configure().

The thing is that the following "kind of" seems to work, because the default ("gsl-lite") causes compile errors with the project I'm trying to use mp-units with. I say "kind of", because I can't find any -DMP_UNITS_API_CONTRACTS=3 anywhere in the compile_commands.json after running the build (nor do I see it in the terminal).

self.options["mp-units"].contracts = "ms-gsl"

I also found this which seems to indicate I'm using the right way to configure the option: conan-io/conan#13467. I should note that I find the whole conan documentation pretty terrible though. A lot of things don't seem to be explained very well if at all.

Edit: All mp-units options (whether set or not set in my conanfile.py's configure()) are available under self.dependencies["mp-units"].options in build(). And they have the proper value.

@mpusz
Copy link
Owner

mpusz commented Jun 4, 2024

Hmmm, strange. compile_commands.json contains all of the compilation flags for me.

@mpusz
Copy link
Owner

mpusz commented Jun 4, 2024

I do not think that you should set the options of dependencies in the Conan configure() function. At this stage, the dependencies graph is not yet established correctly. According to Conan docs, you should set the options either from the profile or command line or by setting the default_options https://docs.conan.io/2/reference/conanfile/attributes.html#default-options, but the last one has some issues.

@AnthonyVH
Copy link
Author

I'll dig more into this later this week.

I'm wondering about something that seem related though. All the examples depend on mp-units::mp-units, which sets all the required defines in src/core/CMakeLists.txt. However, all those defines are defined once again for each example executable (see example/CMakeLists.txt). AFAIK those should be set due to the mp-units::mp-units dependency. Why is this redefinition required?

@mpusz
Copy link
Owner

mpusz commented Jun 5, 2024

The definitions in the example directory apply only to example_utils, which are not dependent on mp-units::core. However, thanks for pointing this out. Initially, it was just one innocent find_package(gsl-lite), but it grew recently. Now it is a lot of copy-paste code. I will try to refactor this to one CMake target and link with it in both places.

mpusz added a commit that referenced this issue Jun 6, 2024
…ition in different separate targets

Refers to #581
@AnthonyVH
Copy link
Author

I've tried a bunch more things, but can't get this to work.

Below is a minimal example that reproduces the issue for me. The std_format option is not passed to mp-units (or if it turns out it is, nothing is done with it). Passing it via the command line (-o 'mp-units/*:std_format=False) doesn't work either. The compiler spews out a bunch of errors about v1 not being formattable and MP_UNITS_API_STD_FORMAT is not defined at the point mp-units/compat_macros.h is included.

Is there something obvious missing here?

-- conanfile.py
from conan import ConanFile
from conan.tools.cmake import CMake, CMakeDeps, CMakeToolchain, cmake_layout
from conan.tools.env import VirtualBuildEnv

required_conan_version = ">=1.59.0"

class HelloConan(ConanFile):
    name = "hello"
    settings = "os", "compiler", "build_type", "arch"
    build_policy = "missing"

    options = { "build_folder": ["ANY"] }
    default_options = { "build_folder": "build" }

    def layout(self):
        cmake_layout(self, build_folder=self.options.build_folder.value)

    def configure(self):
        self.options["mp-units"].std_format = False

    def requirements(self):
        self.requires("fmt/10.2.1")
        self.requires("mp-units/2.2.0@mpusz/testing")

    def generate(self):
        env = VirtualBuildEnv(self)
        env.generate()

        tc = CMakeToolchain(self, generator="Ninja")
        tc.generate()

        deps = CMakeDeps(self)
        deps.generate()

    def build(self):
        cmake = CMake(self)
        cmake.configure()
        cmake.build()
# CMakeLists.txt
cmake_minimum_required(VERSION 3.12)

project(conan-mp-units LANGUAGES CXX VERSION 1.0.0)

set(CMAKE_CXX_STANDARD 23)
set(CMAKE_CXX_STANDARD_REQUIRED TRUE)
set(CMAKE_CXX_EXTENSIONS FALSE)

set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

find_package(fmt REQUIRED)
find_package(mp-units REQUIRED)

add_executable(${PROJECT_NAME} main.cpp)
target_link_libraries(${PROJECT_NAME} PUBLIC fmt::fmt mp-units::mp-units)
// main.cpp
#include <mp-units/format.h>
#include <mp-units/systems/si.h>
#include <fmt/format.h>
#include <iostream>

int main() {
  using namespace mp_units;
  using namespace mp_units::si::unit_symbols;
  constexpr quantity v1 = 110 * km / h;
  std::cout << fmt::format("{}", v1) << '\n';
}

@mpusz
Copy link
Owner

mpusz commented Jun 10, 2024

You are right. There is a bug in package_info() Conan method. I am really not the biggest function of this Conan functionality :-(

I should fix it soon.

@mpusz mpusz closed this as completed in 3d5a636 Jun 10, 2024
@AnthonyVH
Copy link
Author

Thanks a lot for the super fast fix! Looking forward to trying it out.

@mpusz
Copy link
Owner

mpusz commented Jun 10, 2024

I hope it fixes your problem. If not, please feel encouraged to reopen.

@mpusz
Copy link
Owner

mpusz commented Jun 10, 2024

Some bugs were found with the CI. I will fix them in a few minutes.

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

No branches or pull requests

2 participants