Add support for modular build structure. #401
Conversation
|
Please review and merge this PR at your earliest convenience. |
|
I didn't see this, because I am not getting notifications when a PR is opened unless someone mentions me with @HDembinski. I changed something in the options of this repo, putting my email address in explicitly under email notifications. Perhaps that is what was missing. @grafikrobot Thank you for the changes to the build. There seems to be an issue with boost.random in the slow tests, a linker error. Do you have an idea how to fix this? You also remove some extra warning flags |
Those are for avoiding warnings in Math (IIRC) that cause errors because of the warnings-as-errors option. |
Have those slow tests ever passed? Asking as I don't see them in previous commits. |
Yes, they used to pass. I only run them on PRs, but not on pushes, that's why they don't show up in the list of commits. |
|
@HDembinski FYI.. I've been trying to resolve the CI errors. But I can't make progress in most of them myself.
PS. I do notice that the coverage and slow tests have not been working for PRs for some time. Including for this one that I made to investigate the build differences between unchanged develop and my changes: #409 |
|
@grafikrobot Thank you for the work, I will look into this on the weekend. The problem for me also is that I am not developing on a Mac anymore, I have a Windows machine now (urgh). I think I can fix all these three issues. |
|
So, I finally had a closer look.
It looks like If I remember correctly, lcov only works with a matching version of gcc. If you know a better way to measure coverage in C++, I'd love to switch to another tool. You upgraded from gcc-11 to gcc-15, I will probably need to upgrade lcov as well.
That's because of boost-serialization if you look closely, which you added as a constant dependency to this build. boost-serialization causes lots of issues, that's why I keep it as an optional dependency that is only compiled for some tests that are not as strict. I run harder tests on boost-histogram itself.
That's a newly discovered bug - it seems. |
|
@grafikrobot Progress. The coverage test still fails, because it doesn't compile anything. test//all works in other circumstances. Weird. The gcc slow test still fails because of that "possibly dangling pointer". As far as I understand, the pointer shouldn't be dangling because of NRVO. The issue is not detected by another other compiler and not caught by the UBSAN / ASAN tests, so perhaps a false positive. As a workaround, we could turn off that warning for gcc for that line. |
| @@ -16,10 +16,10 @@ target_link_libraries(boost_histogram | |||
| INTERFACE | |||
| Boost::config | |||
| Boost::core | |||
| Boost::math | |||
There was a problem hiding this comment.
Should you remove that to mirror the b2 side where math is optional?
There was a problem hiding this comment.
Maybe, but it does cause so many problems, because it is header only.
This is part of the effort to make the Boost libraries "modular" for build and consumption. See https://lists.boost.org/Archives/boost/2024/01/255704.php and https://github.com/grafikrobot/boost-b2-modular/blob/b2-modular/README.adoc for more information.
This PR depends on the following other PRs being merged to both develop and master branches of the respective repos:
This PR will be changed to ready for review, i.e. not draft, when the above are merged. Do not merge this one until that time.