Add support for building with CMake #215
Comments
Cazadorro
changed the title
|
Tested all tests, the following tests failed to even build
|
chiphogg
changed the title
|
Wow --- thanks for taking this on! It's very exciting to see this progress. This is a big step towards enabling the community to use Au more fully, and will unblock further improvements later such as Conan availability. I see that it was confusing that the tests didn't already pass under MSVC. I should be able to clear that up. We added our MSVC support in #151. Since we didn't have build rules that we knew how to get working for MSVC, what we did instead was to make a single-file version of the library, and run basic rudimentary tests, as in the PR. Despite its simplicity, this approach was already enough to expose some issues with Au on MSVC. We figured we could add more tests to this file if we find more problems. Of course, the ability to run all of the tests (which I think your changes would provide) would be even better! As for the specific failures themselves, I've given a cursory glance. I think your suggestion that it comes down to different definitions of This probably also explains the " The Here's a question: if somebody checks out your branch, how could they run MSVC themselves to iterate on it? I do have access to a Windows computer at home, so maybe I could dig into it at some point. |
|
By the way, I changed the title of this issue so that it could serve as the "support CMake" issue. |
chiphogg
added
📁 kind: enhancement
I'm using CLion, so somethings are automatically handled for me, for example, CLion ships with CMake and Ninja, so I don't have to point to the location of CMake. As a consequence I tested a configuration with vscode which should be easier to acquire. I tested using vscode on my end, and I couldn't get Ninja to work the same as CLion, so I replaced the default generator on windows to be Visual Studio 17 2022 64bit, that seemed to work in vscode. You don't need CMakePresets to make this work, it just makes it way easier to set up. There's probably a better way to do this with CMakeKits, but I haven't used them yet, Visual studio build tools I think make their own kits available. I think the following should be all that is required ignoring vscode.
With out an IDE just from the CLI normally you'd just run (with presets)
which would generate the files needed to build for the specified generator, then you would run
though I think the preset handles the last I'll also explain what I did to set up an environment for VScode as well (don't normally use vscode, so I don't know if I did this "right"). It appears as if vscode was able to figure out where the build tools existed, and didn't require me to install CMake, appeared to point to one installed with visual studio build tools automatically On the VScode side of things, I believe you need to install the following VScode extensions CMake, CMake-tools, and cpptools (which I think goes by C/C++ on vscode now?). Then you will need to press f1 and find "Cmake: select configure preset" and set one of the non-vcpkg configure presets (there's WindowsDebug, WindowRelease, and WindowsDebWithRelInfo). Then select the "triangle with a wrench" icon, and click the folded page with "configure". on the right hand side, this will basically do the equivalent of Again, a lot of this was new to me an hour ago, as I don't use vscode, but hopefully this should allow you to run and execute the code using CMake and also be able to run the debugger. |
|
@chiphogg I made the changes you suggested for units and now at least those tests work. I investigated more in to Given this worked with out the test cases from the very first test group, I wanted to see if I could at least get all the test cases to work if the first test cases weren't uncommented. But I ran into more weirdness. The following works The following doesn't work (again same error below) And the following does work But the following doesn't work and in fact, that last line shown above doesn't work unless the first two above test cases are disabled re-arranging doesn't appears to fix things, if I move it like so and uncomment the first it doesn't compile (with any combination of the first two): But will still compile in combination with the two it would compile with before. I'm not sure what the deal is here. I don't really see how this is GoogleTests fault, since I eliminated all other test groups from this test. I don't see how it's leaving things behind if that's the problem. |
|
Hmm... the spooky action-at-a-distance feels like something we'll need to root cause to get unblocked. I think it's worth putting up a checklist for what it would take to get the CMake support landed.
Just to set expectations, I find I've been swamped at work since coming back from break (unsurprisingly, given Aurora's 2024 goals). So this will be more something for my "personal project" time for the next little while. And on that front, it's competing with writing papers for the C++ Standard Units Library, which is a higher priority for me but is starting to fall behind. So, this is both explanation and apology as to why I don't expect I'll be able to take a crack at setting up the CMake dev environment any time real soon, despite the fact that this is a really exciting and encouraging development --- and that I'm really grateful for what you're doing here. However, there's still lots of value in making these posts! If there are other CMake users who want to help out, this issue's contents give them a huge head start. I hope my checklist will also add clarity about the bigger picture of what it takes to get this landable, and where we're at now. |
chiphogg
added
good first issue
|
@chiphogg Alright, so I had a thought, does the issue still happen if we use and... it compiled. So I undid commenting out everything else, and the only other part that failed to compile was
And it doesn't fail to compile. So... I'm not sure what is going on, but this should work on MSVC and GCC/CLang and still result in the same tests. This made me think it actually was a google test related problem, but even with out their complicated macros... weird things happen. I think it may have something to do with how deep the templates goes with matchers causing some compiler error internally to MSVC with some sort of overflow? So I got prefix_test.cc working, and everything passed. Then I tried to see if the same issue was with quantity_test.cc... and sure enough a single line (where EXPECT_TRUE is below) had to be changed. But it's strange since a similar line in the subsequent function wasn't needed (though it was comparing doubles there)). So quanity_test.cc built. Then I went to quantity_point_test, and sure enough another EXPECT_EQ was causing compile failure (and again, one very similar to it up above didn't have the same issue...) changed to But I ran into an issue with google_test's "matchers" and I got the following error: When a test did the following:
Which made no sense, Where is the kelvins here? I kept moving things around, until I ended up with this mess that finally passed, but I had to completely avoid matchers to do it. The same error will appear even if I try to get around the GTest macro and manually use the underlying matcher stuff. Some how the mere existence of I think the quick solution is possibly manually changing stuff that fails to compile with matchers to use EXPECT_TRUE, but only for MSVC when it is detected (since the test failure errors are just worse with out the matchers used here). If you can, can you report this to https://developercommunity.visualstudio.com/cpp and say something like:
|
|
(Sorry I didn't get to this last weekend) So are you saying that the problems now only happen for CMake and MSVC? I just wanted to get clear on that before reporting a bug to MS. Anyway, from a big picture point of view, here are my latest thoughts. I think changing the test contents is a last resort. I don't want us to be constrained from what kind of tests we can write. We will always have the bazel builds to make sure the code is thoroughly tested. What's more, testing individual helper libraries on CMake is less important (especially when they're robustly tested by other means). I think what we really need to test here is that each individual "target" (I'm using bazel terminology here; I don't know CMake) can be used correctly in a project. So maybe the right approach is to make a folder of CMake tests, and just have some barebones tests for each target to prove that the target can work? |
Yes, though I don't think this is a CMake issue, I'm even using Visual Studio generator (and not ninja) with these issues, so it means that this is identical to if I tried to us msbuild with Visual Studio itself to build this. So there's nothing there that attaches CMake to the utter weirdness of these compile issues, I've only tested this with MSVC, presumably if you didn't have issues before with these tests on GCC or CLang, then neither of those compilers have that problem either, . My understanding from previous correspondence was that all tests passed on GCC and CLang, but only a reduced set of tests were used for MSVC, so what I'm saying now is based off of that information. I don't have a linux system set up at home, but I'll see if I can set up github actions to do this automatically and test with GCC at least, I've not used them before so I'll have to look into how this works, and that will probably take a while.
Targets are more or less the same between Bazel and CMake, CMake derived the terms from Make, and I think Bazel did the same. Infact the targets are almost identical between Bazel and CMake here. That being said, I'm not sure if it makes sense to "test" the targets but I could be wrong. I don't think it's normal to test whether the target itself works or build systems, presumably there isn't a "does bazel work" part of this repo right? By virtue of any of the built test even running, we already know the targets themselves work (au is itself a dependency of the tests, tests all use basically identical target dependencies as Bazel), and would be immediately usable in an Installability is different. but AFAIK this library isn't installable right now with bazel any way. And when I talk about installing the library, I mean using the dedicated instrall command necessary command for your build system, and then the user immediately being able to use the library. Looking at the docs, it currently still requires lots of setup in comparison to properly configured CMake projects. Currently, as I've designed the CMake file, the library should already be usable with I can test vcpkg installability locally, as I have a private registry, but the final port file for VCPKG will need to be submitted to VCPKG itself (which I can also do). This will need to refer to a real tagged git commit version, and has to be manually updated updates (automatic pulling of latest commit can work, but causes issues with binary caching with VCPKG, it's expected ports will have pinned versions available). I believe VCPKG itself has some sort of total binary compatibility CI task running, so technically the port should also be tested there, but triaging adding the port itself should get the issue resolved regardless, all of that happens outside of this repo (including the port support, so long as this repo gets cmake support). |
|
Cool! Let me try to summarize to make sure we're on the same page generally. CMake TestsIn order to land CMake support, we'll need to have some tests that build the library using CMake. We'll also need to run those tests automatically. However, that doesn't mean we have to get all current tests running under CMake. The fact that those tests pass under any build system with a given compiler is enough to give us confidence that the code works for that compiler (I think this is what you were saying in your last comment). We do still need some tests that make sure that the "main" library targets work in their CMake incarnation (these will be the ones that we'll set up CI runs for). But these tests can be smaller, almost more like smoke tests. (https://github.com/aurora-opensource/au/blob/main/release/au_all_units_hh_test.cc is an example of this.) So I think the action here is to set up some barebones CMake tests, and create CI jobs to run them automatically. InstallabilityOnce we get CMake support landed, we'll want to make Au available on conan and vcpkg! In order to do this, it sounds like we'll need to cut a new release. (You mentioned that we could alternatively automatically pull the latest commit, at the cost of having some issues --- but that turns out to not matter, because I strongly prefer to use an immutable tagged and named release anyway.) It seems likely that we'll end up making a 0.3.5 release which is mainly a "CMake support" release, plus perhaps a couple of small improvements that might come along for the ride. After this release gets cut, it sounds like you'll be able to help provide Au on vcpkg. I don't know conan but I'd be willing to try to figure that out. Installability with bazel
Just to be clear: you mean "this library isn't installable right now with bazel for CMake users any way", right? For bazel users the installation process is really smooth AFAIK. SummaryBased on all of this, I think the updated checklist would look something like:
At that point, we could consider this issue resolved. After that, we'd need to cut a new release, and then make that release available on conan and vcpkg. (And then we'd want to update the installation instructions again.) Does that all sound about right? |
I can't tell if there's confusion here or not, so let me clarify some things. CMake is a meta build system, technically, it generates build files for a target system (make msbuild, ninja etc...). I don't think I quite understood that Bazel is different here, my understanding now is Bazel is more akin to a cross platform Make? So when I say "CMake can't be causing X issue", it's because CMake isn't what gets run when your project actually builds, it's the generator chosen with CMake (the compiler and real build system combo), it literally cannot be responsible for many of these types of issues, by definition. My understanding is that all these issues with MSVC are just caused by MSVC itself. On the flip side I consider it a bug if the cmake version of the library isn't able to have complete parity with GCC and Clang in terms of tests. To further clarify there should be no test that Bazel can run period that can't be run under the CMake. If that's the case, it's something I need to fix. To reiterate it was my understanding that the library currently doesn't compile all tests under MSVC with bazel.
All tests should run under GCC and CLang, and my understanding is the tests I have gotten to run with CMake for MSVC is a strictly larger set than was able to compile under the previous configuration with Bazel (though that had nothing to do with Bazel, and now should technically be able to run the same tests through MSVC).
Yes.
I misinterpreted the install instructions and didn't see the end for use with bazel.
This is about right, though I think all tests should be run with cmake that can, the only things right now that can't be run on MSVC are quantity_tests and prefix_tests (though even then only some tests). We could also conditionally enable tests based on |
|
Thanks for helping me fix up my mental model for CMake! You're right: I didn't understand CMake's role as a "meta build system". I have very limited experience with CMake myself. We used it at a former company, but then we switched to bazel after I was there for a year or two, and it was a great relief and I never looked back. I think this new mental model has some strategic implications as well. Namely: I think it makes sense to focus on getting CI jobs up and running for clang and gcc, because those should be able to run all of the existing tests without modification. I could try pushing commits to your branch... but I think it might be easier if we turn it into a PR and try to land it in something like in its present state, without any CI tests. We could add comments at the top to say that it's experimental and unsupported, with a So the revised plan might be something like:
Does this plan sound reasonable? If so, go ahead and make the PR for the first step out of the CMake-related changes in your branch! |
|
Latest updates: I checked out the branch which @Cazadorro mentioned in the first post here, and started playing with it. I'm really CMake-ignorant, but I was able to figure out how to install a new enough version, and build the code. Nice! I wasn't able to figure out how to run the tests, though --- that part was extremely confusing. I did find the test binaries inside of the The comment about CMake being a "meta build system" was very helpful in resetting my mental model, and preparing me to get the most out of tutorials --- thanks for that! So here's my current thinking as to where to go from here.
I specifically want to make sure I'm learning modern CMake. I've found various tutorials, and I think Henry Schreiner's is the most promising. Especially based on this line from the front page:
Might as well start out right! Therefore, here's what I am planning to do.
I'm feeling more confident that I'll be able to write good CMake, now that I have this plan that breaks it into reasonable chunks. That said, there's no substitute for an expert pair of eyes, to spot things I could easily overlook. @Cazadorro, I don't think I can formally add you as a reviewer on these PRs, because you're not part of the aurora-opensource org (and I don't think we can change that). However, I'll try to remember to post a comment with each new PR here (and if I forget, they'll be linked automatically anyway). Feel free to leave comments on any PR, before or after landing. Also, feel free to do nothing! You've already been immensely helpful in kick-starting this effort. Having your branch as a "cheat sheet" will absolutely turbo-charge this effort! 🚀 I don't know a good ETA for completion, but I expect to be able to get the first PR or two out within the next two weeks. |
|
Sorry life got in the way and I completely forgot about this, though I'm glad you're taking the dive into learning CMake. Here's some things you should know:
I've used that site myself, but IMO, the "best" resource on learning CMake is unfortunately https://crascit.com/professional-cmake/ by the same person featured in the earlier CPPCON presentation. I say unfortunate because it's both paid (30 dollars), and they also are a "CMake co-maintainer" thus... have direct oversight and control to fix CMake's notorious documentation and learnability problems... and simply haven't... while selling a book that apparently does so... Despite that it's the most comprehensive resource out there and is kept up to date to my knowledge, even about making things installable.
I screwed up and forgot to put |
We start by creating the top-level `CMakeLists.txt` file, following the modern CMake guide (https://cliutils.gitlab.io/modern-cmake/). We already have a `build/` directory, so we'll use `cmake/build` for our build folder. We add this to `.gitignore` (using a `*` so as to support multiple build folders). We update the release instructions so as to keep the version number in sync, and the installation instructions so as to explain how to use CMake. Helps #215.
|
Life does tend to do that. 🙂 Here's the first PR, single-purpose focused on laying the foundations (#254). @Cazadorro, if this looks good, I can get a blocking reviewer to review it. The next PR would add a target for |
|
@chiphogg Looks good to me! |
|
Thanks! For future reference, you're more than welcome to comment on the PRs themselves, which would make it easier to give feedback for future, presumably-less-trivial PRs. I just can't actually add you to the list-of-official reviewers, that's all. 🙂 |
We start by creating the top-level `CMakeLists.txt` file, following the modern CMake guide (https://cliutils.gitlab.io/modern-cmake/). We already have a `build/` directory, so we'll use `cmake/build` for our build folder. We add this to `.gitignore` (using a `*` so as to support multiple build folders). We update the release instructions so as to keep the version number in sync, and the installation instructions so as to explain how to use CMake. Helps #215.
|
I'm reading through the Professional CMake book. I think I need to pay attention to the packaging and installation sections. Currently, I'm thinking it's related to the fact that we never call |
|
@chiphogg There are two typical to consume a cmake library. As an installed library (so either you or some package manager calling CMake install) where the files are placed in some platform/package manager defined location, or as a subdirectory, where the library isn't installed at all, but instead uses the top level CMakeLists.txt. To get access to properly configured installed libraries, you use find_package(package_name) To use subdirectory cmake projects, use add_subdirectory("path/to/cmakelist.txt") and the targets exposed there will be available to the top level cmake and exposed to subsequent cmake files called in You typically avoid add_subdirectory if you've got access to a package manager, but it's bad practice to have it a project that is usable as a subdirectory but not as an installed library, or vice versa. It's pretty obvious why you'd want to have a library be installable, but if you don't have it usable in add_subdirectory, some systems that don't support package managers or in some cases offline systems, will have a hard time using that package. If you're not doing anything super weird (this project doesn't) and you follow how the professional cmake book demonstrates how to make an installable library, your project should be this way by default. The two big things are to make sure aliased targets (ie Au::target) are also exposed in the top level cmake and not just created at install, and that find_package dependencies also obey this rule, or you check manually for those targets existence before calling find_pacakge if they don't (find_package will work on targets which have been properly configured and acquired through add_subdirectory instead of actually being installed on the system). The latter is typically a problem with projects that massively change naming convention at INSTALL_INTERFACE vs BUILD_INTERFACE, which happens way more often than you'd think (one is capital, one is lower case, or adds numbers etc...) |
|
I have to pack up for a trip, and I just saw that you left this message, after I spent the last couple hours trying to get it to work! I'll read it later today or tomorrow, but meanwhile, #265 (draft!) is my progress report. The PR summary contains repro instructions. On the off chance that I'm really close, let me know if I've missed something obvious. Otherwise I'll read the message when I get a chance and try further. |
|
Ok, I caught up on the message. We're stopped for a bit so I can write a
real reply. :)
First take: installing feels like the hardest, most complicated step yet. I
think that's at least in part because I've spent my whole software career
in monorepos, so third party dependencies didn't need to be "installed",
just brought into the repo (although I guess this is a kind of installation
for that monorepo project). I've been reading and rereading the relevant
sections of Professional CMake, hoping the concepts will sink in.
As for where we're at with the draft PR: I think that *finding the include
path* to use inside the dependent project's files might be the only thing
left to do. Before I added all the commands in that PR, I was getting CMake
errors about nonexistent targets. I *think* we have a real target now...
but I have no idea where the files are properly available.
As for the methods of installation: I'm hoping that the "fetch content"
method in the PR summary is a good stand in for find_package(). If that's
right, then I think our tasks are to (a) figure out how to get that example
to work, and (b) get an add_subdirectory() example to work, and then we'll
have everything we need to write the usage instructions in the docs!
…On Fri, Jul 12, 2024, 3:42 PM Chip Hogg ***@***.***> wrote:
I have to pack up for a trip, and I just saw that you left this message,
after I spent the last couple hours trying to get it to work!
I'll read it later today or tomorrow, but meanwhile, #265
<#265> (draft!) is my
progress report. The PR summary contains repro instructions. On the off
chance that I'm really close, let me know if I've missed something obvious.
Otherwise I'll read the message when I get a chance and try further.
—
Reply to this email directly, view it on GitHub
<#215 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAN4IYGIAA4XQ2DFVDJWMODZMAWT5AVCNFSM6AAAAABBK4RERKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRWGI2DOMRVGY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
We may have gotten the project to _build_ under CMake, but _other_
projects can't actually _use_ it unless we provide `install` commands.
First, we add an `install(TARGETS...)` command (based on the
`GNUInstallDirs` module) to the `header_only_library()` function. We
install it as part of an export set, whose name we set with a global
variable (which must be defined before we load this file).
The tricky bit is that this can't work unless the actual code itself is
in a subdirectory of the repo, _not_ the root folder, for reasons I only
dimly understand. To make this work, we now provide the Au code via a
symlink, which we create inside of a new `cmake/project_symlinks`
folder. This lets us refer to this folder in the `BASE_DIRS` variable.
In the main CMake file, we add the installation command for the export
set we've been building up. We also write and install the package
config and version files.
The above changes would break bazel support, because there would be
BUILD files reachable underneath `cmake`, both via the `au` symlink, and
via `cmake/build`. To fix this, we make `cmake/` a fake local
repository, causing bazel to skip it.
To test, create a new project with two files:
`main.cc`:
```cpp
#include "au/au.hh"
int main(int argc, char **argv) { return 0; }
```
`CMakeLists.txt`:
```cmake
cmake_minimum_required(VERSION 3.29)
project(CppUnitsCompare)
include(FetchContent)
FetchContent_Declare(
Au
GIT_REPOSITORY https://github.com/aurora-opensource/au
GIT_TAG "chiphogg/cmake-install#215"
)
FetchContent_MakeAvailable(Au)
add_executable(CppUnitsCompare main.cpp)
target_link_libraries(CppUnitsCompare PUBLIC Au::au)
```
And execute:
```sh
cmake -S . -B build
cmake --build build
```
Helps #215.
For now, we simply run "the default CMake generator on the latest Ubuntu". Later on, if we tighten up our notion of "supported CMake configurations", we can add more options here. Helps #215. --------- Co-authored-by: Tim Hirsh <[email protected]>
This replaces the `project_symlinks` approach we had been using for CMake: it turns out that symlinks are fraught and error prone on Windows. To make this work, we had to satisfy a number of challenging constraints: 1. The include path had to be `"au/..."` (e.g., `"au/io.hh"`) for all files, on both bazel and CMake. 2. The `au` folder that contained the actual code needed to live inside of a true subdirectory --- not the git repository root folder (as it has been since time immemorial), and not a symlink (as we had been using to get CMake support). 3. The git repository root folder needed to also be the root of the bazel folder, because we use that for all of our bazel-backed tools, and because we want users to be able to run all `bazel` commands from the repo root. I previously tried a variety of approaches here, including: - Having CMake create the symlink, instead of having it checked in (bad idea: you can't create symlinks in Windows without developer mode, and even after I enabled dev mode it didn't work). - Making a `code` subfolder in the git repository root that had _its own separate `WORKSPACE` file_, and was included as a local bazel repo (terrible idea: we'd need two copies of all auxiliary bazel files, and making the single-file script work was nightmarishly complex --- I never even fully got there). In the end, the approach that works is to use the `includes` attribute of the `cc_library` rule. This is simple and direct: it's designed to solve this exact problem. The only real downside is that we need to prepend `code/au` to a lot of files in the `BUILD.bazel` for `//au`, and also in a couple of places in the single file script. Helps #215.
|
The finish line is in sight!
After that, it'll be time to cut the 0.3.5 release. 😎 |
This replaces the `project_symlinks` approach we had been using for CMake: it turns out that symlinks are fraught and error prone on Windows. After this PR, CMake support on Windows is confirmed to work! To make this work, we had to satisfy a number of challenging constraints: 1. The include path had to be `"au/..."` (e.g., `"au/io.hh"`) for all files, on both bazel and CMake. 2. The `au` folder that contained the actual code needed to live inside of a true subdirectory --- not the git repository root folder (as it has been since time immemorial), and not a symlink (as we had been using to get CMake support). 3. The git repository root folder needed to also be the root of the bazel folder, because we use that for all of our bazel-backed tools, and because we want users to be able to run all `bazel` commands from the repo root. I previously tried a variety of approaches here, including: - Having CMake create the symlink, instead of having it checked in (bad idea: you can't create symlinks in Windows without developer mode). - Making a `code` subfolder in the git repository root that had _its own separate `WORKSPACE` file_, and was included as a local bazel repo (terrible idea: we'd need two copies of all auxiliary bazel files, and making the single-file script work was nightmarishly complex --- I never even fully got there). In the end, the approach that works is to use the `includes` attribute of the `cc_library` rule. This is simple and direct: it's designed to solve this exact problem. The only real downside is that we need to prepend `code/au` to a lot of files in the `BUILD.bazel` for `//au`, and also in a couple of places in the single file script. Helps #215.
This lets us support installing the library to the system, and then using it via `find_package()`. To test this, I ran `sudo cmake --install cmake/build`, observed that it completed with no errors, and then made a new CMake package that simply used `find_package(Au)`. Everything worked! Helps #215.
This lets us support installing the library to the system, and then using it via `find_package()`. To test this, I ran `sudo cmake --install cmake/build`, observed that it completed with no errors, and then made a new CMake package that simply used `find_package(Au)`. Everything worked! Helps #215.
To test, I ran `au-docs-serve`, and did an A/B comparison with respect to the currently deployed doc website. Looks like the table got updated correctly, the tabbed instructions work, and everything else was as expected. Fixes #215.
|
@chiphogg Hopefully this is fine to ping in this thread, but I wasn't sure where this should go.
I just found this thread today: mpusz/mp-units#518 and it's possible they are experiencing similar issues (even though it's seemingly c++20 related) with MSVC, weird issues that are hard to nail down with out the example being "complicated" enough. If Microsoft fixes this it may help the issues here. However In the cpp reddit, one of the MSVC standard library maintainers said that compiler work is currently not a priority at Microsoft "but that should change soon", if you look there's very little support for C++23 language features and basically zero work on C++26 (which I don't think is done yet, but both GCC and Clang managed to complete most of the current features). I'm not sure when this will change or if we're entering a new era of MSVC being way behind. |
|
Yep, this is definitely a good place to ping. I'm a watcher on mp-units, so I did see that thread. It hadn't occurred to me to connect it to our issues. In any case, it'd be lovely if MSVC could find and fix these issues. 🙂 |
We have one target, Also, for broader context, the team is definitely bazel-first in terms of our day-to-day experience. We're excited to support CMake and conan, but since we don't use them on a regular basis, we're likely to get things wrong or make surprising choices out of ignorance and inexperience. 🙂 So please take that into account, and feel free to offer guidance on best practices in these domains! |
|
So from the CMake point of view, is there a reason why you want to split This being just headers I don't understand why you need to explicitly declare that the |
|
I've created this rough PR to illustrate what I meant #298 |
This is really valuable feedback. Thanks! It shows me some things I hadn't even considered. And I think I can explain why things are the way they are. The short answer is that our bazel libraries are that way because in bazel, it actually matters --- due to sandboxing, everything's hermetic, so it keeps our dependency tree clean. And our CMake libraries are that way because we simply ported them from bazel! But they don't have to stay that way. On the bazel size, it's critically important to me that the Au library is composed out of many, well-defined, single-purpose targets ( For CMake, though, you've helped me realize that it doesn't need to be this way! The fact that this structure exists in bazel is enough to keep the library structure clean. I can see how it would be convenient to have a single target for the library --- or, perhaps, two, with a separate target for the libraries that depend on GTest. When I read this post, I had been planning to explore this route. I still am, but I see you've posted an (explanation-only) PR, so I'll read that first to learn what I can. |
I have the fork here: https://github.com/Cazadorro/au/tree/au_cmake Note currently I set cmake standard to C++20, but that is for debugging build issues, I'll set it back to c++14 when it's ready.
Basically right now I'm attempting to get this library to work with cmake. I'm going case by case trying to get this library to work with cmake with different tests.
Some of these tests straight up would never work under MSVC, for example, using
M_PI, when#define _USE_MATH_DEFINESis not used. It gave me compile errors, so I had to add code to actually get some tests to compile under MSVC. I also don't see any MSVC configs with Bazel, which is doubly confusing, I was under the impression the library was tested against msvc based on this from the readmeNow I'm getting issues with
math_test, here's the first error dump of my compile output:Entire version at end.
I'm not sure what the deal is here, these compile errors imply there's some sort of specialization that isn't happening? If I was missing a header, it should have already complained about it in a separate error, but I'm not seeing that happening. I know MSVC has some issues with CRTP and constexpr members, but I don't see that happening here either? And given the above CI tests I would think that this would have been caught if it was a legitimate incompatibility with MSVC?
This test appears to be causing the issue, but there are some weird nonsensical successful compiles and failed compiles in this library with some of the other tests that fail, where I've removed and added tests back in, so I'm not sure how much stock I'd put into this being the issue.
in particular, if this line remains in the code, and everything below is commented out, the tests fails,
constexpr auto celsius_origin = clamp(celsius_pt(0), kelvins_pt(200), kelvins_pt(300));However, when everything above is removed, the test compiles fine... and passes... if QuantityPointConsistentWithStdClampWhenTypesAreIdentical and if QuantityPointProducesResultsInCommonUnitOfInputs are the only test remove, the test also passes. If either of those two are not commented out, it doesn't work. If I remove QuantityPointTakesOffsetIntoAccount and comment everything below, things work.
This has already taken more time than I anticipated, I don't think I'll be able to swing trying to also get this tested under clang or gcc right now, I was hoping I could get some help identifying what the issue is.
Entire log for math_test
<\details>
The text was updated successfully, but these errors were encountered: