mpusz / mp-units

The quantities and units library for C++
https://mpusz.github.io/mp-units/
MIT License
994 stars 79 forks source link

Configuration not propagated to compiler defines #581

Closed AnthonyVH closed 3 weeks ago

AnthonyVH commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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: https://github.com/conan-io/conan/pull/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 commented 4 weeks ago

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

mpusz commented 4 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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.

AnthonyVH commented 3 weeks ago

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 commented 3 weeks ago

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.

AnthonyVH commented 3 weeks ago

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

mpusz commented 3 weeks ago

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

mpusz commented 3 weeks ago

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