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

fmt v11 and c++20 modules support #10

Merged
merged 37 commits into from
Sep 11, 2024

Conversation

kamrann
Copy link
Contributor

@kamrann kamrann commented Jul 18, 2024

Updates to address issues in modules support provided by the build2 package. See issue #5.

@kamrann
Copy link
Contributor Author

kamrann commented Jul 20, 2024

Notes and various issues encountered so far:

  1. It appears the upstream modularization of fmt is currently making assumptions of more than just c++20, since it doesn't wrap includes such as <expected> in preprocessor conditionals within the module source file, as it does with the same includes in the header mode. This leads to a warning on MSVC unless using both /std:latest (cxx.std=experimental) and latest preview 14.41.
  2. MSVC will error out when building fmt module unless cc.reprocess = true is set. This looks like it's probably a bug in MSVC when combining /P with a private module fragment. Unfortunately cc.reprocess = true combined with modules appears to be somewhat bugged in build2 generally, clean builds are ok but iterative builds keep generating errors about missing preprocessed source files.
  3. clang with libc++ on Windows looks like it just doesn't work with fmt, modules or no, some issue relating to interpretation of compiler defines and FMT_MSC_VERSION . Needs more investigation, though not a priority of course.
  4. clang with MSVC on Windows also giving errors about internal linkage, didn't dig further.
  5. The fmt module source file is in /src. Since it's an interface unit though, I think this means we'll need to tweak the install behaviour in the buildfile to install this source file. Obviously all the interface stuff is in the header, but since BMIs aren't installed, I'd assume downstream consuming build tools working with the installation would need access to the source file in order to rebuild it?

@kamrann
Copy link
Contributor Author

kamrann commented Jul 21, 2024

Re. 2
build2 behaviour with reprocess=true: build2/build2#409
MSVC /P and private module fragments: https://developercommunity.visualstudio.com/t/Separate-preprocessing-with-P-fails-wit/10707183

@kamrann
Copy link
Contributor Author

kamrann commented Jul 21, 2024

Some more experimenting. 4. also applies with clang on Linux. I'm unclear on where the bugs are, but I've filed an issue with both LLVM and fmt.

kamrann added 23 commits July 22, 2024 17:35
…the module fix PRs, but they're yet to make an upstream release.
Add gtest and gmock dependencies via build2 packages.
Begun to map the logic from upstream CMake; added static library test-main and some initial tests to a basics subfolder.
Removed install module from fmt-tests bootstrap.build.
… rather than format_parse_context alias which is not exported by the fmt module.
@kamrann kamrann marked this pull request as ready for review September 10, 2024 06:08
Copy link
Collaborator

@Klaim Klaim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, most of my comments are for clarifications

@@ -0,0 +1,45 @@
include ../test-main/

#test_names =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these comments still useful?

# args-test \
# assert-test \
# base-test
gtest_test_names = args-test assert-test base-test chrono-test color-test compile-fp-test compile-test format-test gtest-extra-test noexception-test os-test ostream-test printf-test std-test unicode-test xchar-test
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this list have been formed from globing the .cc files? If not consider adding a comment about where to look to update this list (in upstream/tests/CMakeFiles.txt)

# base-test
gtest_test_names = args-test assert-test base-test chrono-test color-test compile-fp-test compile-test format-test gtest-extra-test noexception-test os-test ostream-test printf-test std-test unicode-test xchar-test

# header-only-test <- header only mode seemingly not supported by this build2 package?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it was not supported, I basically had the same reasons as explained in the packaging guide although that wouldnt prevent support of header-only mode later if the demand is there. So far nobody needed it.

import libs += fmt%lib{fmt}
import libs += gtest%lib{gtest} gmock%lib{gmock}

liba{test-main}: test/cxx{test-main gtest-extra util} test/hxx{gtest-extra mock-allocator test-assert util} $libs
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would defining it as libu would have helped usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I'm yet to understand the intended usage of libu*, despite having read through the docs information on them many times. When/why do you choose to use it?

I also found this: Note also that if you plan to link any of your unit tests in the whole archive mode, then you will also need to exclude the source file containing the primary executable's main() from the utility library
which suggests that if used here then test-main library could not contain text-main.cc. Having said that, the test-main library target is essentially reflecting upstream CMake and it's arguably not even worth having?

@@ -1,9 +1,13 @@
import! [metadata, rule_hint=cxx.link] libs = fmt%lib{fmt}

./ : exe{driver} : {cxx}{driver} hxx{tests.inl} $libs testscript{**}
./ :
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that line still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently not ;)
I may have thought it more clear since the following two are both conditional, but indeed seems superfluous.

@kamrann kamrann merged commit 74ed2cb into build2-packaging:master Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants