-
Notifications
You must be signed in to change notification settings - Fork 31
Update Exemplar build to be modules-first #167
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
Conversation
0466eb7 to
307fb4c
Compare
|
Well the good news is the lint passes. Most of the test failures are on compilers that don't support modules and the MSVC ones are... something |
307fb4c to
10f35cd
Compare
I'm going to be clapping the performance personally :) |
| os: ubuntu-latest | ||
| toolchain: "cmake/gnu-toolchain.cmake" | ||
| cpp_version: 17 | ||
| cpp_version: 20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too me this is consistent with other discussions -- it's simply too taxing on library authors to support below 20. We can leave that to boost...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we had this discussion last year at sync.
The consensus is to leave minimum support version undefined but still encourage libraries to support lower version if possible.
The big argument for 20 was concept support, the big argument for below companies like Adobe is still using 17 and we want their usage experience.
Exemplar can support 17 (hell, it can probably support 11, or even 99!), so to signify this we made exemplar support 17.
I think we can revisit this once 26 comes out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consensus is to leave minimum support version undefined but still encourage libraries to support lower version if possible.
Consensus can be changed. And, technically I'm not against leaving the minimum undefined, but my personal recommendation to library authors is to not bother with versions below 20. Concepts, ranges, and a bunch of other things are just too consequential in building a good modern library. We have limited intellectual capital here and time wasted on supporting older version isn't applied to building a better future. Remember, we're in the pivot to work on c++29 libraries. And for 29 don't be surprised if we see people requiring 26 for reflection and contracts.
Let me back this up with some data, however. According to the 2024 survey, C++20 is not allowed at all in 37% of projects. The year over year progress from 2023, was in the 6% range -- from 43% not allowed in 2023 to 37%. About 50% answered that they expected to be able to upgrade this year. But conservatively I expect we'd be in the 30% exclusion range and dropping fast. When you note that 8% are still excluded from C++17 this is really only a 22% exclusion. This is all reasonable as far as I'm concerned -- the slow adopters will eventually get there and our focus here is on the future, not the past.
As a side note, the barriers to adopting 20+ with an older code base are small in my experience. We've continued to break very little legacy code so for lots of stuff it's simply changing the compiler flags and moving on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah looks like good points, take this to the sync.
I don't think most libs can support versions below C++20 anyway.
I think you basically have to convince david on this.
| ) | ||
|
|
||
| add_subdirectory(src/beman/exemplar) | ||
| if(BEMAN_EXEMPLAR_INSTALL_CONFIG_FILE_PACKAGE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is exemplar and all except you are learning cmake -- maybe a comment to explain the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is exemplar and all except you are learning cmake
From previous PRs, I don't think this is the intention. CC @camio
10f35cd to
18fc27f
Compare
|
I've pinged @wusatosi about the CI failures. The root of it is despite appearing to run on many different compilers, actually all the GNU builds run on GCC 13 (which doesn't support modules) and all the LLVM builds run on Clang 18 (which doesn't support modules unless you turn off language extensions, which we don't go out of our way to do). I haven't investigated the MSVC failure yet. So the CI has been lying a little bit for a long time. If you wanted to test your code on GCC 13 and Clang 18 though it worked really good. Very tested. |
Good catch -- obviously no one is looking closely at the CI lol |
Weird, I have manually tested this before. Edit: This issue was introduced by #159 . Edit2: PR up #168 |
|
CI fail is not only because proper version is not selected. Please check compiler test seris, e.g. llvm 18 CI test. See: https://github.com/bemanproject/exemplar/actions/runs/15358684463/job/43222580631?pr=167#step:5:69 Edit: okay I am mistaken, clang 18 does not support modules. |
| workflow_dispatch: | ||
| schedule: | ||
| - cron: '30 15 * * *' | ||
| - cron: "30 15 * * *" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch, I think we actually want to change them all the single qutote because github expressions does not support double quote...
that's definitely topics of another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm for this refactoring, but can we make this a single PR, it's unclear exactly what has been changed.
And in scope PR what I did is check the compiler versions and exclude anything that isn't clang-20 or g++-15 (not yet in CI). I don't have clang19 setup yet, so I wasn't confident modules would work there. And related note -- scope CI is actually testing other compilers and versions. |
Thanks for checking, the bug is introduced in #159 which as far as I know isn't used anywhere yet.
This is out of scope, but if we want usage experience we probably should not be this aggressive on using the latest compilers. |
I setup clang++-19 and have now included it in the modules supporting compilers since it appears to work fine.
We can only support modules usage on compilers that are capable of supporting modules (that's probably around 4 at the moment). The non module users can have a broader range of compilers. Also, while I understand your advocacy for earlier c++ versions and feature sets, I will continue to remind you that it's not the focus of Beman. Again, let Boost do that -- they've self selected on that -- we're about the future, not the past. I expect much more in the way of libraries using advanced compilers as reflection and contracts get compiler support. Which by it's very nature excludes everyone below 26. |
I definitely support this.
While I understand supporting 17 maybe not achievable for most beman library (inplace vector doesn't support it either), I do think it is very useful for libraries to support lower C++ versions, e.g. C++20 for right now. In a few years, you can argue for C++23. But if we are always only supporting the latest compilers, latest C++ versions, it will limit us on how we gather usage experience.
User feedback is literally in our mission:
|
|
We can do a dumb thing where we flag-fork the examples to use |
You want the exampmes to use While I want to support using modules, I wouldn’t want to force them. I that sense I think it would be good if Beam libraries do support modules but using them via headers should also be possible. |
|
I want examples and tests to use modules where feasible, yes, because that's the only way to ensure the full API is being properly exported by the interface unit. Being usable via headers is just as important, but given that the typical interface unit is an include of the headers followed by a bunch of |
|
Just to double check, this is meant to be merged into mainline, not only a POC right @nickelpro |
|
If we want all the things that this PR does, it's meant to be merged. If we don't want all the things it does, it's an illustration for the general approach and proving out things like the CI problems. |
While I would not discourage a highly motivated developer, I will also say that if you are emulating concepts for a library proposal with SFINAE or other metaprogramming techniques, you still also need to implement the standard ones that are specified in Constraints and Mandates clauses, or you are not really implementing the standard proposal. That's not a criticism of the quality of a library, or the utility of having it available at $job. But it isn't the evidence I want to see that the specification is correct---and getting the specification correct is tricky. Moreover, it's also very likely that eliminating the features enabled by C++20 and up can make the implementation, and even the specification, worse. The related question of how far to go with downlevel compilers is not very far. Again, putting into use is a goal, but if gcc-12 doesn't work, so much worse for gcc-12, unless the fix is very trivial. This might not be the best place to discuss policy, but it is a place. |
|
My two cents is that libraries that aim to be a part of the standard have a secondary duty to dogfood the standard. If modules, or concepts, or pattern matching, or whatever, are totally unworkable in the implementation of a standards proposal, that is something the people who go to committee meetings should have first hand experience with. To that end, using modern compilers with the newer standard features is good source of nutritious kibble. |
| COMPONENT beman.exemplar | ||
|
|
||
| FILE_SET CXX_MODULES | ||
| DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this intended to install the cppm files to include tree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There has been ongoing arguments about where they should go, since they aren't files that one should #include, but there are a two packages that have the module file in include, vulkan and gcc-15 if I recall correctly, so there's some precedent. It doesn't really matter terribly as long as the metadata file for the module points to the location so the module can be compiled in the context of the consumer, with the consumers compiler, flags, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They exist outside the bounds of what is defined by the FHS. In spirit they are closest to header files, being architecture-independent interface descriptions, so they go in include.
I would forget Also on OSX see https://gitlab.kitware.com/cmake/cmake/-/issues/25965#note_1575723 I use |
|
It's not super likely CMake will support spins of a compiler which change default behavior like that. The bug should go upstream to the Homebrew cask, they shouldn't be modifying the install such that clang needs help to find its own resources. |
Can we have the module-first be trigger by a compile option? e.g. have 2 build modes
And we'll duplicate these 2 examples from examples to have
TBD, I also think we should have tests with module-version and non-module version. |
For development workflows you need to build and test both of them. I'm not sure about packaging workflows. For super-project workflows it probably is just a matter of which targets are depended on, and should be transparent to the consumer? I hope, or we have deeper issues for the build system. |
There's no reason for or advantage to such a division beyond being able to lower the required CMake/compiler versions for the packaging machine. For consuming of the package there is no advantage whatsoever. I'm not opposed in-principle, but every option should have a motivating use case for why it exists. |
A single target supports both workflows, a header-based consumer need not even be aware that a module is available and vice-versa. |
I think we have 3 different things:
I had a similar discussion with @JeffGarland yesterday and it seems to share the same opinion. |
This is just one mode. There is no semantic difference between with and without modules. I'm asking for a use case so I can explain this more practically, "I want to be able to do X".
All the examples will be identical except for a single line, the one that imports the module or includes the header. I do not think the examples should be replicated to demonstrate a single line can be changed. This can maybe be a configuration option, but honestly what's wrong with just leaving a comment like this PR does?
This doesn't do anything, the exact same code is run. The coverage is identical. |
That is not possible: bash-5.2$ cmake --workflow appleclang-debug --fresh
Executing workflow step 1 of 3: configure preset "appleclang-debug"
-- The CXX compiler identification is AppleClang 16.0.0.16000026
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- The C compiler identification is AppleClang 16.0.0.16000026
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Looking for __cpp_lib_ranges
-- Looking for __cpp_lib_ranges - found
Examples to be built: identity_direct_usage;identity_as_default_projection
-- Configuring done (8.6s)
CMake Error in CMakeLists.txt:
The target named "beman.exemplar" has C++ sources that may use modules, but
the compiler does not provide a way to discover the import graph
dependencies. See the cmake-cxxmodules(7) manual for details. Use the
CMAKE_CXX_SCAN_FOR_MODULES variable to enable or disable scanning.
-- Generating done (0.0s)
CMake Generate step failed. Build files cannot be regenerated correctly.
bash-5.2$ |
|
There is an additional problem with gest, if locally installed, the version is not right checked: |
|
@nickelpro as you noted in a review, it the project must be build without presets! bash-5.2$ git diff CMakeLists.txt
diff --git a/CMakeLists.txt b/CMakeLists.txt
index a241e5e..23563d2 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-cmake_minimum_required(VERSION 3.28)
+cmake_minimum_required(VERSION 3.28...4.0)
+set(CMAKE_PROJECT_TOP_LEVEL_INCLUDES cmake/use-fetch-content.cmake)
project(
beman.exemplar # CMake Project Name, which is also the name of the top-level
# targets (e.g., library, executable, etc.).
bash-5.2$ |
|
In a suitably provisioned environment, yes. However, how the user chooses to provide GTest is none of our business. They may have already downloaded it from vcpkg or the Ubuntu repos, or installed it from source. If they need one-click builds without regards for dependency management, then they use presets. You should never The issue you're referencing broke tests with idiomatic CMake usage in a correctly provisioned environment. |
|
@nickelpro sorry, but the find_package() without And with an other You see the linker errors on OSX with cat {
"dependencies": [
{
"name": "googletest",
"package_name": "GTest",
"git_repository": "https://github.com/google/googletest.git",
"git_tag": "6910c9d9165801d8827d628cb72eb7ea9dd538c5"
}
]
}
|
|
I wouldn't generally provide a
The correct answer is to rely on the shipped dependency manifest(s) to provision the correct versions of dependencies, trying to verify that after the fact is a redundancy that can only lead to errors. I didn't write the Downstream packagers shouldn't know anything about project dependency manifests, or the CML, or anything. The Debian maintainer assigned to package your library doesn't want to read any of those things and does not care. They want a ReadMe that gives a set of package names and compatibility ranges, which they will pull from their own Debian repos when packaging your project. Same goes for the Arch Linux TU, the vcpkg contributor, the Conan recipe author, etc, etc, etc. They all have their own dependency sources they're going to use, dependency manifests are irrelevant to them and they do not want to learn a hundred different manifest formats and build system conventions. The correct way to talk to packagers about dependencies is a bulleted list in the ReadMe. |
|
For the AppleClang failure: Yes, as mentioned above it makes packaging with AppleClang impossible. AppleClang can still consume an installed I mentioned this when I said:
I'm not sold on my own position here. It might be reasonable to allow compilers which don't support modules to package a module library. I don't love it though, because the downstream consuming the package has no mechanism to infer this particular build of I'm thinking on it, I might have a hack for Apple Clang. |
|
With the Bulgaria meeting and everything else going on I'd lost track of this PR -- looks like this is mostly ready to go? |
ClausKlein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do not disable the macOS platform!
|
No, this is not ready. As discussed CMake needs to do the right thing on platforms where modules are not available, principally AppleClang but also older compilers. I'll rework this to perform "the right thing" and then push for review and merging. |
|
Closing out this PR since there hasn't been activity since July. Feel free to reopen if there's interest in continuing it in the short term. |
This is a big block of changes (well, as big as it gets with exemplar) that does two things:
beman.scopeto be packageable, as well as discussions had at C++Nowbeman.exemplarto be "modules-first", which is to say it requires module support from the toolchain in order to run tests and build examples, but still supports using the traditional header files when installed as a libraryThis does not violate anything in the Beman standard, but rather makes use of things we might want to have concrete consensus on now that they've arisen as issues.
I do not intend for this to be merged outright, it's more a piece of performance art. It's easier to debate pros and cons with a concrete implementation of all those pros and cons laid out.
Related Discussions: