feat: add C++ modules support

[JohelEGP]

Resolves #7.

[gitpod-io]

[mpusz]

Huge work! Thank you!!!

[mpusz]

It seems that this is no longer a header-only library, right? It will produce different artifacts for different compilers. So probably we should get rid of the: https://github.com/mpusz/units/blob/70586138d9590076a3b46b12133956cf97dcfc41/conanfile.py#L145-L146 in case modules are being used (additional Conan option needed).

[JohelEGP]

Just not "only". The header-only library targets are still there, and are even used by mp-units::module through its linked library mp-units::systems.

[mpusz]

Yes, but as long as there is at least one artifact that is compiler-dependent, Conan has to generate a different ID for such configurations. This is why we need a module-specific ON/OFF option in Conan and when it is on we should not use self.info.header_only() as this forces generation of the same identifier for the package (no matter what the compiler settings are).

[JohelEGP]

I see. Do I just need to remove those lines?

[mpusz]

Yes, add a new Conan option and make a branch that will run it only when modules are not being used.

[mpusz]

Also please add docs here: https://mpusz.github.io/units/usage.html#conan-options for both Conan and CMake.

[mpusz]

@JohelEGP, is this PR ready to merge (besides merge conflicts)? I thought that you still wanted to add to it.

[JohelEGP]

https://github.com/Kitware/CMake/blob/v3.27.4/Help/dev/experimental.rst#c20-module-apis
explains how to use CMake's experimental support for C++ modules.
Change the v3.27.4 in the link for your CMake version.

Using Clang 18, I'm afraid there seems to be a bug:
It was because I was using Clang modules (for import std; and #include translation).
Otherwise, it seems to work fine.

Error with Clang modules

/home/johel/Documents/C++/Forks/mpusz/units/test/unit_test/static/quantity_test.cpp:52:22: error: constraints not satisfied for class template 'quantity' [with R = mp_units::si::metre{{}}, Rep = short]
   52 | static_assert(sizeof(quantity<si::metre, short>) == sizeof(short));
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/quantity.h:85:28: note: because 'RepresentationOf<short, get_quantity_spec(mp_units::si::metre{{}}).character>' evaluated to false
   85 | template<Reference auto R, RepresentationOf<get_quantity_spec(R).character> Rep = double>
      |                            ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:82:28: note: because 'short' does not satisfy 'Representation'
   82 | concept RepresentationOf = Representation<T> && ((Ch == quantity_character::scalar && is_scalar<T>) ||
      |                            ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:79:93: note: because 'short' does not satisfy 'Scalable'
   79 | concept Representation = (is_scalar<T> || is_vector<T> || is_tensor<T>)&&std::regular<T> && detail::Scalable<T>;
      |                                                                                             ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:70:3: note: because 'short' does not satisfy 'CastableNumber'
   70 |   CastableNumber<T> ||
      |   ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:65:62: note: because 'std::common_type_t<short, std::intmax_t>' (aka 'long') does not satisfy 'ScalableNumber'
   65 | concept CastableNumber = CommonTypeWith<T, std::intmax_t> && ScalableNumber<std::common_type_t<T, std::intmax_t>>;
      |                                                              ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:62:3: note: because 'std::regular_invocable<std::multiplies<>, long, long>' evaluated to false
   62 |   std::regular_invocable<std::multiplies<>, T, U> && std::regular_invocable<std::divides<>, T, U>;
      |   ^
/home/johel/root/clang-main/bin/../include/c++/v1/__concepts/invocable.h:34:29: note: because 'invocable<std::multiplies<void>, long, long>' evaluated to false
   34 | concept regular_invocable = invocable<_Fn, _Args...>;
      |                             ^
/home/johel/root/clang-main/bin/../include/c++/v1/__concepts/invocable.h:28:3: note: because 'std::invoke(std::forward<_Fn>(__fn), std::forward<_Args>(__args)...)' would be invalid: no matching function for call to 'invoke'
   28 |   _VSTD::invoke(_VSTD::forward<_Fn>(__fn), _VSTD::forward<_Args>(__args)...); // not required to be equality preserving
      |   ^
/home/johel/root/clang-main/bin/../include/c++/v1/__config:805:17: note: expanded from macro '_VSTD'
  805 | #  define _VSTD std
      |                 ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:71:24: note: and 'typename T::value_type' would be invalid: type 'short' cannot be used prior to '::' because it has no members
   71 |   (requires { typename T::value_type; } && CastableNumber<typename T::value_type> &&
      |                        ^
/home/johel/Documents/C++/Forks/mpusz/units/src/core/include/mp-units/bits/representation_concepts.h:73:24: note: and 'typename T::element_type' would be invalid: type 'short' cannot be used prior to '::' because it has no members
   73 |   (requires { typename T::element_type; } && CastableNumber<std::remove_reference_t<typename T::element_type>> &&
      |                        ^

[JohelEGP]

This is how I tested this (using CPM instead of Conan).
In the CMake buildsystem:

CPMAddPackage("gh:gsl-lite/gsl-lite#4b5e9ab7474841fc2d7efc2e0064ef81785535d1")
CPMAddPackage("gh:fmtlib/fmt#e8259c5298513e8cdbff05ce01c46c684fe758d8")
CPMAddPackage("gh:catchorg/Catch2#3a5cde55b7a27a6d94ce994a8362b2425211bd5e")
execute_process(COMMAND "${CMAKE_COMMAND}" -S "${CPM_Catch2_SOURCE}" -B Catch2
                WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}" COMMAND_ERROR_IS_FATAL ANY)
set(Catch2_DIR "${CMAKE_CURRENT_BINARY_DIR}/Catch2")
list(APPEND CMAKE_MODULE_PATH "${CPM_Catch2_SOURCE}/extras")
CPMAddPackage(
  NAME "mp-units"
  VERSION "modules"
  GITHUB_REPOSITORY "JohelEGP/mp-units"
  OPTIONS "MP_UNITS_USE_LIBFMT TRUE"
  SOURCE_SUBDIR "src")
add_subdirectory("${CPM_mp-units_SOURCE}/test" "mp-units-test")
add_subdirectory("${CPM_mp-units_SOURCE}/example" "mp-units-example")

[mpusz]

This is how I tested this (using CPM instead of Conan).

We need to be able to trigger this from Conan.

[mpusz]

Hi @JohelEGP, it would be great to merge this PR but there are still some unresolved issues we discussed above.

[JohelEGP]

All that's left is the integration with Conan.
I'll leave that to you.

I suppose you're going to add an option to Conan, which translates to a CMake option.
You should use that in place of CMAKE_EXPERIMENTAL_CXX_MODULE_CMAKE_API.

https://github.com/fmtlib/fmt/releases/tag/10.1.1 is needed for the fixes for C++ modules.
Of #7 (comment), llvm/llvm-project#54574 is still open, so warnings abound.

[JohelEGP]

Unsurprisingly, GCC 13 and GCC 14 ICE.

[mpusz]

Should we export everything (even the detail namespace) or things like detail_unit that explicitly states:

mp-units/src/core/include/mp-units/unit.h

Lines 242 to 246 in a356db7

	* @note User should not instantiate this type! It is not exported from the C++ module. The library will
	* instantiate this type automatically based on the unit arithmetic equation provided by the user.
	*/
	template<detail::DerivedUnitExpr... Expr>
	struct derived_unit : detail::expr_fractions<detail::is_one, Expr...> {};

[JohelEGP]

Ideally, not.

[mpusz]

The CI needs fixing as well. We should probably add modules tests to CI as well to check if there are no issues in the PR. Which compilers should the module-build support as of now? Only clang-16+?

When you are done with all the required changes and the CI will be fixed I will contribute to this PR with Conan-related changes to enable CI builds with modules.

[kobi-ca]

piggyback on this issue. I'm trying to run CPMAddPackage like this:

CPMAddPackage("gh:mpusz/mp-units#f3b18e055fc96fb0cbe85fdbb96a39dac369051b")

and getting

.../build/_deps/mp-units-src/CMakeLists.txt'
  is meant to be used only as a CMake entry point and should not be included
  from other CMake files.  Include
  '.../build/_deps/mp-units-src/src/CMakeLists.txt'
  directly instead.
Call Stack (most recent call first):
  build/_deps/mp-units-src/CMakeLists.txt:40 (ensure_entry_point)

what am I doing wrong?

thanks!

[mpusz]

Hi, the problem is that any solution that uses CMake's add_subdirectory for dependencies is not the proper way to do the dependencies management in C++. I spoke about it in one of my talks: https://youtu.be/mrSwJBJ-0z8?t=1931.

As stated in our FAQ it is not only about tests and examples. There are many issues related to this approach.

I strongly recommend using a proper package manager like Conan. Alternatively, to fix your issue, you have to find a way to do add_subdirectory on the ./src subdirectory rather than the main one in the repo.

[mpusz]

This project's directory structure is described in our docs.

[kobi-ca]

This project's directory structure is described in our docs.

thanks. I can start working on bringing in conan for our project.

I tried this small test (just as a quick try before bedtime):

# https://stackoverflow.com/questions/72362303/how-to-use-fetchcontent-populate-with-eigen
cmake_minimum_required (VERSION 3.12)

project (FetchContentExample)

# download CPM.cmake
file(
  DOWNLOAD
  https://github.com/cpm-cmake/CPM.cmake/releases/download/v0.38.3/CPM.cmake
  ${CMAKE_CURRENT_BINARY_DIR}/cmake/CPM.cmake
  EXPECTED_HASH SHA256=cc155ce02e7945e7b8967ddfaff0b050e958a723ef7aad3766d368940cb15494
)
include(${CMAKE_CURRENT_BINARY_DIR}/cmake/CPM.cmake)

CPMAddPackage("gh:gsl-lite/gsl-lite#bd9eb162d42d8ae6a7e86902ca7060247d71ac41")
CPMAddPackage("gh:fmtlib/fmt#7.1.3")


include (FetchContent)

FetchContent_Declare(
  mp-units
  GIT_REPOSITORY https://github.com/mpusz/mp-units.git
  GIT_TAG        v2.2.0
)

FetchContent_GetProperties(mp-units)
if(NOT mp-units_POPULATED)
  FetchContent_Populate(mp-units)

  add_subdirectory(${mp-units_SOURCE_DIR}/src ${mp-units_BINARY_DIR}/src EXCLUDE_FROM_ALL)
endif()

But I'm getting export issue. Not sure if I want to fight it :)

...
-- Configuring done (4.7s)
CMake Error: install(EXPORT "mp-unitsTargets" ...) includes target "mp-units-core" which requires target "fmt" that is not in any export set.
CMake Error in build/_deps/mp-units-src/src/CMakeLists.txt:
  export called with target "mp-units-core" which requires target "fmt" that
  is not in any export set.

[mpusz]

This is one of the reasons that I recommend using true package managers. mp-units has its own dependencies as stated in our docs. Those may depend on other packages as well. If you do not want to resolve all of this manually I strongly recommend using a proper package manager like Conan.

A short intro to Conan can be found here https://mpusz.github.io/mp-units/latest/getting_started/installation_and_usage/#obtaining-dependencies. Please note that mp-units 2.2 was just released yesterday and it may take a few days for it to be added to ConanCenter.

[mpusz]

If you want to use the latest stable package, the easiest way would be to follow: https://mpusz.github.io/mp-units/latest/getting_started/installation_and_usage/#conan-cmake-live-at-head.