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

Import BOOST 1.84. #16621

Merged
merged 4 commits into from Apr 17, 2024
Merged

Import BOOST 1.84. #16621

merged 4 commits into from Apr 17, 2024

Conversation

bangerth
Copy link
Member

@bangerth bangerth commented Feb 10, 2024

First draft. Probably many things still wrong with this.

In reference to #16618.

Fixes #16662.

@bangerth bangerth added this to the Release 9.6 milestone Feb 10, 2024
@bangerth bangerth force-pushed the boost-1.84 branch 2 times, most recently from fb9b042 to 42bb6bd Compare February 10, 2024 18:35
@bangerth
Copy link
Member Author

Well, it works! At least on everything but the Windows platform:

  vector_tools_project_qpmf.cc
D:\a\dealii\dealii\bundled\boost-1.84.0\include\boost/smart_ptr/detail/sp_counted_base_gcc_atomic.hpp(35,5): error C3861: '__atomic_fetch_add': identifier not found [D:\a\dealii\dealii\build\source\numerics\object_numerics_debug.vcxproj]
D:\a\dealii\dealii\bundled\boost-1.84.0\include\boost/smart_ptr/detail/sp_counted_base_gcc_atomic.hpp(40,12): error C3861: '__atomic_fetch_sub': identifier not found [D:\a\dealii\dealii\build\source\numerics\object_numerics_debug.vcxproj]
D:\a\dealii\dealii\bundled\boost-1.84.0\include\boost/smart_ptr/detail/sp_counted_base_gcc_atomic.hpp(49,31): error C3861: '__atomic_load_n': identifier not found [D:\a\dealii\dealii\build\source\numerics\object_numerics_debug.vcxproj]
D:\a\dealii\dealii\bundled\boost-1.84.0\include\boost/smart_ptr/detail/sp_counted_base_gcc_atomic.hpp(58,13): error C3861: '__atomic_compare_exchange_n': identifier not found [D:\a\dealii\dealii\build\source\numerics\object_numerics_debug.vcxproj]
D:\a\dealii\dealii\bundled\boost-1.84.0\include\boost/smart_ptr/detail/sp_counted_base_gcc_atomic.hpp(67,12): error C3861: '__atomic_load_n': identifier not found [D:\a\dealii\dealii\build\source\numerics\object_numerics_debug.vcxproj]
D:\a\dealii\dealii\bundled\boost-1.84.0\include\boost/smart_ptr/detail/spinlock_gcc_atomic.hpp(48,16): error C3861: '__atomic_test_and_set': identifier not found [D:\a\dealii\dealii\build\source\numerics\object_numerics_debug.vcxproj]
D:\a\dealii\dealii\bundled\boost-1.84.0\include\boost/smart_ptr/detail/spinlock_gcc_atomic.hpp(61,9): error C3861: '__atomic_clear': identifier not found [D:\a\dealii\dealii\build\source\numerics\object_numerics_debug.vcxproj]

It's not clear to me why it would look at the GCC headers. Anyone see what I'm doing wrong?

@marcfehling
Copy link
Member

This issue was reported earlier in #16413.

@bangerth
Copy link
Member Author

Does someone have a Windows platform and could see how we end up in these header files? The errors only report that we do, but it would be very useful to see the include chain if someone was willing to dig a little bit...

@marcfehling
Copy link
Member

Maybe not directly related, but potentially we run into the same problem?
microsoft/vcpkg#27450

@bangerth bangerth force-pushed the boost-1.84 branch 2 times, most recently from 452a514 to 30ea0dd Compare April 6, 2024 00:45
@bangerth
Copy link
Member Author

bangerth commented Apr 6, 2024

OK, this finally seems to work.

This is ready to merge if you all agree with one design decision: In the past, the BOOST copy we have locally was a stripped down version that only included a subset of BOOST (a moderately curated superset of what we actually use). This has the advantage of saving ~80MB of space in the repository: the entire set of BOOST header files + the small number of source libs we ship is ~180MB, whereas what we used to package is ~100MB. On the downside, this means that if you relied on the bundled version of BOOST, you couldn't use all of BOOST. It is also quite a lot of work to strip down a BOOST version to the level we used in the past, because inevitably newer versions of some component of BOOST rely on other components they did not in the past.

My proposal here is to just package all of the header files. That's not all of BOOST: we only package the serialization, system, and iostream libraries, but BOOST has quite a few more. But I think it's still a better situation. Opinions?

@bangerth
Copy link
Member Author

bangerth commented Apr 6, 2024

Tests correctly now.

Copy link
Member

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Fine with me.

Copy link
Member

@kronbichler kronbichler left a comment

Choose a reason for hiding this comment

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

I am ok with it as well; I am curious about the possible impact on the size of the git meta data, especially the compressed history. Do we have an estimate of how much bigger the download from a git clone is going to be?

@bangerth
Copy link
Member Author

bangerth commented Apr 8, 2024

@kronbichler I don't know. How would I test this?

We have, over the past 25 years, repeatedly imported BOOST versions that were 80-100 MB large. I suspect that importing one more at 180 MB is not going to break the bank. But as a point of comparison: If you take the 180 MB in the boost/ headers directory, and you .tar.gz it, you get a file that has 16 MB. Perhaps git doesn't do it quite as efficiently and ends up with something that is twice the size -- let's round that up to 40 MB. For comparison, my .git directory currently has a size of pretty much exactly 450 MB. So it's a bump of 10% at worst.

@kronbichler
Copy link
Member

I imported all you branches, including this one, and the size of my .git directory increased by less than 50MB (298 -> 347 MB). If I git gc --aggressive, my size reduces to around 248 MB, so indeed git can figure out that boost also evolved as a repository with mostly moderate changes between 1.70 and 1.84. So in that sense 👍 👍 from my end, the size increase is very moderate.

@vdilecce
Copy link

The Boost header file modified by e969195 was added in Boost 1.74.

I am guessing therefore that on MSVC any Boost 1.74+ will result in deal.II failing to compile: this is troublesome for scenarios where an external Boost installation is used unless this hack is manually replicated beforehand.

This worries me, but I also understand this looks like Boost's fault, not deal.II's. I wonder if this shall be at least documented for users configuring deal.II with an external Boost 1.74+ on MSVC.

@masterleinad
Copy link
Member

Let's see what the boost smart_ptr developers upstream have to say, see boostorg/smart_ptr#112.

@bangerth
Copy link
Member Author

I found the issue. It's due to Kokkos defining things starting with a double underscore: boostorg/smart_ptr#112 (comment)

Kokkos should simply not be doing this.

@masterleinad
Copy link
Member

Kokkos should simply not be doing this.

Yes, fixed in Kokkos 4.1.00 (kokkos/kokkos#5804).

@masterleinad
Copy link
Member

We can decide if we want to try to patch the bundled Kokkos or rather keep the Boost patch.

@vdilecce
Copy link

In both cases, configuring deal.II with an external installation of the internally patched library would require the same manual patch to be applied to the external installation beforehand (unless configuring with an external Kokkos 4.1, which changed licensing though!)

If one assumes it is more frequent to configure deal.II with an external Boost than with an external Kokkos, patching the bundled Kokkos would probably cause less trouble.

@masterleinad
Copy link
Member

If one assumes it is more frequent to configure deal.II with an external Boost than with an external Kokkos, patching the bundled Kokkos would probably cause less trouble.

See #16882.

@bangerth
Copy link
Member Author

My suggestion would be to keep the patch here, just in case.

@bangerth
Copy link
Member Author

Ping?

@kronbichler
Copy link
Member

Given that this has been open for a while without additional comments, and that it has received two approvals, I will take the decision and merge now.

@kronbichler kronbichler merged commit ae6ebd0 into dealii:master Apr 17, 2024
28 checks passed
@bangerth bangerth deleted the boost-1.84 branch April 17, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue of Boost std::unary_function on ARM-based mac with Clang 15
5 participants