Add a test for the module #2283

DanielaE · 2021-05-13T12:07:10Z

add an option to build {fmt} as module
add a module use test
add a module fail test

This is mostly just the groundwork. The module build rule supports only msvc 16.10-pre3 or later so far, none of which isn't yet available in CI. Others can chime in and add support for gcc or clang.

I'm a total cmake noob and this is the first time I did anything cmake-related beyond running away from it. If anybody has a better way to express the compile-dependency from the BMI generation to TUs importing that BMI this should be improved. I'm worn out and annoyed by all the restrictions and the lack of expressiveness.

Real tests to check {fmt}'s API in the module use test will follow in future PRs.

vitaut

Thank you for the PR!

CMakeLists.txt

vitaut · 2021-05-16T00:38:09Z

CMakeLists.txt

+    set(FMT_BMI "${CMAKE_BINARY_DIR}/fmt.ifc")
+    set(BMI_BUILD_FLAGS /interface /ifcOutput ${FMT_BMI})
+    set(BMI_USE_FLAGS /reference "\"fmt=${FMT_BMI}\"")
+    endif ()


Indent is off.

Yeah. That's also about to change. My current private code is different and I thought about generating these variables by functions that contain all the compiler-specific knowledge on how to create the required compiler options and syntax from a name and a location given as function arguments. This function can then be used to both create a module when the user consumes {fmt} that way and also in the tests where the library must be compiled in both modes at the same time: as a traditional library and a module (only for testing purposes) with a name different from the traditional library. This way all existing functional tests remain as they are and can reach into implementation details as they please. The additional tests for module-capable compilers will not interfere. They don't need to (and also can't) test the implementation aspect again but rather look at the public, user-facing API, possible regressions reagarding unwittingly exposing implementation details like namespace detail and contents of it, and (at least in the beginning) the salient properties of modules and their compiler-specific implementations.

Do you have an opinion about this?

I think this function should take a target name and set the required properties instead of setting a bunch of variables, something like:

function(enable_modules name) if (MSVC) target_compile_options(${name} PRIVATE /interface /ifcOutput ${CMAKE_CURRENT_BINARY_DIR} INTERFACE /reference ${name}=${CMAKE_CURRENT_BINARY_DIR}/${name}.ifc) ... endif () endfunction()

Note that ${CMAKE_CURRENT_BINARY_DIR} is probably not needed because that will be the current directory anyway.

Very good idea! You are definitely more fluent in CMake than I am.

This seemingly unnecessary ${CMAKE_CURRENT_BINARY_DIR} is required to generate correct compiler options. Checked with both the ninja and the vcproj generator.

CMakeLists.txt

vitaut · 2021-05-16T00:41:51Z

CMakeLists.txt

+
+set(FMT_INTERFACE_UNIT src/fmt.cc)
+if (FMT_MODULE)
+  set(FMT_SOURCES ${FMT_INTERFACE_UNIT})


I think you can replace ${FMT_INTERFACE_UNIT} with src/fmt.cc and remove FMT_INTERFACE_UNIT. The only other place that uses FMT_INTERFACE_UNIT should probably use FMT_SOURCES instead.

I can change this into inline text. FMT_SOURCES won't work in the tests where both a traditional library and a module must be compiled. Please see my other comment on this.

vitaut · 2021-05-16T00:43:39Z

CMakeLists.txt

@@ -257,6 +282,20 @@ target_include_directories(fmt-header-only INTERFACE
  $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>
  $<INSTALL_INTERFACE:${FMT_INC_DIR}>)

+if (FMT_CAN_MODULE AND FMT_TEST)
+  set (tstmod "modfmt")
+  add_library(${tstmod} STATIC ${FMT_INTERFACE_UNIT} ${FMT_HEADERS})


What is this target for?

The additional module besides the traditional library.
I want to move this target into the other CMakeLists.txt in the test subdirectory. Hence the idea of generating the required compiler-specific compile options by CMake functions rather than hardcoding them. If at all possible, I want to turn the test-module target into a OBJECT library if there is a way to introduce a dependency from the test-module-related object artifact to the invocation of the compile step of the module tests where the BMI must be available for consumption. So far I haven't found a way to express this. Either the syntax doesn't provide a way to get at the path to the object artifact, or CMake complains about a dependency on the BMI artifact. In the latter case it doesn't accept it because it either has no clue about how to create this artifact or it simply refuses to accept the file extension. In the former case there doesn't seem to exist a reliable and portable way to get at the full path, CMake generator expressions aren't supported in the required syntactical constructs. I need a "proxy" for the actual BMI and this is either a library name at a known path that I can then reliably resynthesize, or the object artifact with the unknown path. So far I go with this test-only library.

I don't think we should be building another module from the same source. Not doing this also eliminates the need in the bmi_vars function.

This is not about compiling "another module". It is about compiling a module at all to enable testing the module-related stuff. The test must be conducted in "traditional lib" mode to make all of the existing tests work. {fmt} is compiled as it ever was. Because the module-based tests cannot excercise those parts of the current functional tests that need access to internal, non-exported parts of the library, {fmt} better be compiled so for testing. Therefore the need for a test-only additional module binary hidden away in the test subdirectory. And because it can no longer clash with the Real Stuff® in the test's root directory the name of the binary is fixed now.

Makes sense now, thanks for clarification. I would recommend adding a comment explaining this.

vitaut · 2021-05-16T00:45:22Z

CMakeLists.txt

-  set(INSTALL_TARGETS fmt fmt-header-only)
+  if (FMT_MODULE)
+    set(INSTALL_TARGETS fmt)
+  else()
+    set(INSTALL_TARGETS fmt fmt-header-only)
+  endif()


I suggest simplifying this a bit:

set(INSTALL_TARGETS fmt) if (NOT FMT_MODULE) set(INSTALL_TARGETS ${INSTALL_TARGETS} fmt-header-only) endif()

Ok. TBH, I haven't even looked at the install part at all besides providing something. I'm interested in the test aspect.

May be I shouldn't even bother with that in this PR at all.

I would suggest leaving the install targets as before unless there is some specific problem with them.

test/CMakeLists.txt

vitaut · 2021-05-16T01:19:48Z

test/module-compile-fail.cc

+import fmt;
+
+// the detail namespace must be invisible
+using namespace fmt::detail;
+
+#if defined(FMT_HIDE_MODULE_BUGS) && defined(_MSC_FULL_VER) && _MSC_FULL_VER <= 192930035
+// bug in msvc 16.10-pre3:
+// the namespace is visible even when it is neither
+// implicitly nor explicitly exported
+#  error namespace 'fmt::detail' is visible!
+#endif
+
+int main() {}


I think it would be better to make it a normal test case in module-test. You can detect absence of fmt::detail just using lookup mechanisms, something like:

namespace fmt { //namespace detail {} } namespace detail { int test_detail_namespace; } namespace fmt { void test() { using namespace detail; int success = test_detail_namespace; // fails to compile if fmt::detail is visible } }

Well, mulling over this suggestion and playing with it a little I have to admit that I don't understand its benefit. May be I'm just dense.

In both implementations, the compiler will stop with a hard error in one of both possible outcomes:

in my implementation it causes a hard error if everything is fine and the conditional compilation part will substitute it with an #error directive if namespace fmt::detail erroneously happens to be visible. The test is supposed to fail compilation and ctest will flag module-compile-fail as "failing" if the compilation succeeds despite the expectation.
The other, 'normal' module-test is supposed to compile. The visibility of fmt::detail is benign in the sense that it doesn't preclude clients from using the module in full. So it shouldn't affect the other module-related tests.

in your suggested implementation, the compile failure is not handled by the test infrastructure if namespace fmt::detail is visible and no other test in module-test is run. I don't think this kind of testing behaviour is helpful.

I've been looking for a SFINAE-friendly option to check for the visibility of a namespace with unknown contents, but there doesn't seem to be one. After all, given a conforming implementation (which msvc 16.10-pre3 isn't and 16.10 likely won't be, too !) this test is meant to defend contributors from unwittingly exposing supposedly inaccessible implementation details. Namespace fmt::detail is one of them, afaik there are more. I'm not sure if a check for these additional namespaces like dragonbox or safe_duration_cast is necessary.

The benefit is that the "test" is moved from CMake to normal C++ code. CMake tests are slow and hard to maintain. It's not different from any other compile error so it's fine if other tests are not run.

Ok, the support for compile-fail checks is removed, too.

vitaut · 2021-05-18T23:12:34Z

CMakeLists.txt

+
+set(FMT_CAN_MODULE OFF)
+if (CMAKE_CXX_STANDARD GREATER_EQUAL 20 AND
+  (MSVC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 19.29.30035)) # msvc 16.10-pre3


Parentheses around the second AND look unneeded. Also please break the long line e.g. by moving the comment to a separate line.

vitaut · 2021-05-18T23:18:58Z

test/module-compile-fail.cc

+import fmt;
+
+// the detail namespace must be invisible
+using namespace fmt::detail;
+
+#if defined(FMT_HIDE_MODULE_BUGS) && defined(_MSC_FULL_VER) && _MSC_FULL_VER <= 192930035
+// bug in msvc 16.10-pre3:
+// the namespace is visible even when it is neither
+// implicitly nor explicitly exported
+#  error namespace 'fmt::detail' is visible!
+#endif
+
+int main() {}


The benefit is that the "test" is moved from CMake to normal C++ code. CMake tests are slow and hard to maintain. It's not different from any other compile error so it's fine if other tests are not run.

DanielaE · 2021-05-19T16:29:46Z

Ok, no problem on my side with your suggested change.

From my CTest runs, I can not see an exceptionally slow test at all, though. Almost all examples that I could find regarding how to implement compile fail test are slow, this implementation isn't:

Test project Z:/fmt
      Start  1: args-test
 1/18 Test  #1: args-test ........................   Passed    0.01 sec
      Start  2: assert-test
 2/18 Test  #2: assert-test ......................   Passed    0.01 sec
      Start  3: chrono-test
 3/18 Test  #3: chrono-test ......................   Passed    0.02 sec
      Start  4: color-test
 4/18 Test  #4: color-test .......................   Passed    0.01 sec
      Start  5: core-test
 5/18 Test  #5: core-test ........................   Passed    0.01 sec
      Start  6: gtest-extra-test
 6/18 Test  #6: gtest-extra-test .................   Passed    0.01 sec
      Start  7: format-test
 7/18 Test  #7: format-test ......................   Passed    0.02 sec
      Start  8: format-impl-test
 8/18 Test  #8: format-impl-test .................   Passed    0.01 sec
      Start  9: locale-test
 9/18 Test  #9: locale-test ......................   Passed    0.01 sec
      Start 10: ostream-test
10/18 Test #10: ostream-test .....................   Passed    0.01 sec
      Start 11: compile-test
11/18 Test #11: compile-test .....................   Passed    0.01 sec
      Start 12: printf-test
12/18 Test #12: printf-test ......................   Passed    0.01 sec
      Start 13: ranges-test
13/18 Test #13: ranges-test ......................   Passed    0.01 sec
      Start 14: scan-test
14/18 Test #14: scan-test ........................   Passed    0.01 sec
      Start 15: module-test
15/18 Test #15: module-test ......................***Failed    0.01 sec
      Start 16: module-compile-fail
16/18 Test #16: module-compile-fail ..............***Failed    0.03 sec
      Start 17: posix-mock-test
17/18 Test #17: posix-mock-test ..................   Passed    0.01 sec
      Start 18: os-test
18/18 Test #18: os-test ..........................   Passed    0.02 sec

89% tests passed, 2 tests failed out of 18

Total Test time (real) =   0.26 sec

The following tests FAILED:
	 15 - module-test (Failed)
	 16 - module-compile-fail (Failed)

vitaut · 2021-05-19T20:32:11Z

I can not see an exceptionally slow test at all, though.

Running tests is fine but it also requires building a separate target. It's not much in absolute terms but these numbers add up quickly since we have a lot of CI configs. Having to deal with CMake code is even more problematic.

vitaut · 2021-05-20T17:25:09Z

CMakeLists.txt

+endif ()
+if (FMT_TEST AND FMT_MODULE)
+  # The tests require {fmt} to be compiled as traditional library
+  message(STATUS "Testing is incompatible with build mode 'module'.")


Who needs tests anyway =).

vitaut · 2021-05-20T17:26:36Z

Thank you!

DanielaE marked this pull request as draft May 14, 2021 12:37

DanielaE force-pushed the feature/add-module-test branch from 3e88071 to c2a78d3 Compare May 14, 2021 14:24

vitaut requested changes May 16, 2021

View reviewed changes

DanielaE force-pushed the feature/add-module-test branch 2 times, most recently from c04eb25 to 036f799 Compare May 18, 2021 17:57

vitaut reviewed May 18, 2021

View reviewed changes

Add a test for the module

8f6164e

DanielaE force-pushed the feature/add-module-test branch from 036f799 to 8f6164e Compare May 20, 2021 12:54

DanielaE marked this pull request as ready for review May 20, 2021 13:06

vitaut reviewed May 20, 2021

View reviewed changes

vitaut merged commit 5a0d99f into fmtlib:master May 20, 2021

DanielaE deleted the feature/add-module-test branch May 20, 2021 17:27

twoixter mentioned this pull request May 29, 2021

Does not compile with CMake version < 3.7 because of "VERSION_GREATER_EQUAL" #2320

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a test for the module #2283

Add a test for the module #2283

DanielaE commented May 13, 2021

vitaut left a comment

vitaut May 16, 2021

DanielaE May 16, 2021

vitaut May 18, 2021

DanielaE May 19, 2021

DanielaE May 20, 2021

vitaut May 16, 2021

DanielaE May 16, 2021

vitaut May 16, 2021

DanielaE May 16, 2021

vitaut May 18, 2021

DanielaE May 19, 2021

vitaut May 19, 2021

vitaut May 16, 2021

DanielaE May 16, 2021

vitaut May 18, 2021

vitaut May 16, 2021

DanielaE May 16, 2021

vitaut May 18, 2021

DanielaE May 20, 2021

vitaut May 18, 2021

DanielaE May 19, 2021

vitaut May 18, 2021

DanielaE commented May 19, 2021

vitaut commented May 19, 2021

vitaut May 20, 2021

DanielaE May 20, 2021

vitaut commented May 20, 2021

Add a test for the module #2283

Add a test for the module #2283

Conversation

DanielaE commented May 13, 2021

vitaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielaE commented May 19, 2021

vitaut commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaut commented May 20, 2021