-
Notifications
You must be signed in to change notification settings - Fork 283
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
update custom easyblock for Boost to always build single and multi threaded versions #2456
update custom easyblock for Boost to always build single and multi threaded versions #2456
Conversation
Still not warning about ignoring boost_multi_thread... Not sure it's really needed... |
This of course need more testing... |
|
||
if self.cfg['boost_mpi']: | ||
self.log.info("Building boost_mpi library") | ||
self.build_boost_variant(bjamoptions + " --with-mpi", paracmd) | ||
mpi_bjamoptions = " --with-mpi" | ||
self.build_boost_variant(self.bjamoptions + mpi_bjamoptions, self.paracmd) |
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 goal should be to do a single invocation of b2 with everything. To use mpi you can add using mpi ;
to the user-config.jam file. I'll double check if that is the same
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, didn't get that to work with my first tries, so I decided to PR it as it is to start with...
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 not so sure about that. Think Boost.MPI the same way we have Boost.Python, then we would in reality only want it to build the mpi parts, i.e. using --with-mpi, but it would then be the only build/install step it does, just like for the python part.
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, but that is not what it currently is: boost_mpi = True
in the EC additionally installs Boost.MPI and we can't change that until EB 5.x
However it makes sense providing a (new) knob to build only Boost.MPI (same as Boost.Python) because we moved Boost to GCCcore, i.e. no MPI there.
We might even have boost_mpi = None
being the default and the EasyBlock decides whether to build with or without MPI based on the currently loaded modules/dependencies.
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.
We could simply check if name = Boost.MPI do --with-mpi and only that (and verify that usempi = True first)
Else if either boost_mpi=True (requires usempi=True) or just usempi = True then add "using mpi" to config and build everything, perhaps verifying that we have MPI in the toolchain in all cases.
usempi=True/False should be the determining factor for MPI since that's what is mostly used in other easyblocks and for simpler easyconfigs.
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.
Nah, I'd prefer not to change this any more, it does the "same" thing as the old one, i.e. multiple b2 invocations when boost_mpi=True, producing all the libs.
It's better to deal with this MPI stuff in a separate PR.
I.e. my Boost-1.74.0-gompi-2020b.eb which has boost_mpi=True produces all the libs of standard boost + the mpi ones and creates the correct links when using this boost.py.
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.
Hm, but multiple invocations of b2 was the source of the problem that lead to this. So IMO we should fix that together. I can offer to tackle the remaining issues on top of this. But yeah, maybe in a separate PR, I guess doesn't matter much
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.
No multiple invocations was not really the problem, it was doing it with layout=system once and layout=tagged once. Now I always do layout=tagged so there is no problem with multiple invocations.
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 mean: if we do a single invocation, then there is no room for such mistakes at all.
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 agree that this can be changed in a follow up PR, let's get this one merged as is to avoid dragging it along...
easybuild/easyblocks/b/boost.py
Outdated
lib_mt_suffix += '-x64' | ||
|
||
shlib_ext = get_shared_lib_ext() | ||
lib_glob = 'lib*%s.%s.%s' % (lib_mt_suffix, shlib_ext, self.version) |
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 replace and glob stuff here gives me headaches. Not sure if all is correct.
Currently we have the following:
- libboost_type_erasure.a
- libboost_type_erasure-mt-x64.a
- libboost_type_erasure-mt-x64.so
- libboost_type_erasure-mt-x64.so.1.74.0
- libboost_type_erasure.so
- libboost_type_erasure.so.1.74.0
So I would say the glob should be lib*.*
and the replacement simply: "use current name but remove everything from first '-' until first dot".
This removes the need for the lib_mt_suffix and the arch stuff above and makes it way simpler to understand: "Remove the tag"
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.
When doing threading=single,multi --layout=tagged you get:
libboost_type_erasure-mt-x64.a
libboost_type_erasure-mt-x64.so@
libboost_type_erasure-mt-x64.so.1.74.0*
libboost_type_erasure-x64.a
libboost_type_erasure-x64.so@
libboost_type_erasure-x64.so.1.74.0*
and nothing else, and that's what the glob/replace deals with.
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.
and nothing else
The "currently" was referring to current EB installations of Boost.
Ah, forgot the single-threaded. Then glob for lib*-mt*.*
instead. Then 2 comments like "Glob for the mt libs" and "remove the tags from the name" would be enough and the code much easier to understand. Maybe even include those names of yours as examples in 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.
yeah, testing...
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.
Better now I hope...
Simplyfy lib globbing and replacement for default libs Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
… by Alexander Grund <Flamefire@users.noreply.github.com>
There seem to be problems with older Boost versions that need to be looked at a bit more... |
@Flamefire Boost 1.71 (at least) doesn't protect itself correctly in the multiple-variants case:
Note that, for 1.71, it's just a warning so the cmake correctly finds all components. |
Yes but the wrong ones: It uses the single threaded ones which would be a breaking change and likely wrong.
We need the tagged layout or it would be a breaking change in EB. However I think we should make the single-threaded build opt-in and clearly warn for known bad versions. Could you attach the 2 files ( Edit: Ok, found it: The multi-thread default was introduced in 1.73: boostorg/boost_install@c7a4270 |
By single build above I mean dropping threading=xxx and -layout=... which would produce the same libraries as we used to have, currently checking which version starts supplying cmake files and the -x86 suffix repsectively. |
The threading=multi --layout=tagged option was added to boost.py in commit e2e4de1 back in 2017. But 1.74.0 is the first easyconfig which actually turned on boost_multi_thread. |
Ah you are right, we always build with system layout and without threading set anyway. Given that the
--> New options |
Note that 1.70.0 is the first version with cmake files included. |
…o only add default links when appropriate.
@Flamefire @boegel How does this look? |
Btw, unless I made a mistake in testing, this produces the exact same result as before for everything up to 1.68.0, from 1.69 it produces an identical result but with tagged layout by default. Still have to finish my changes changes to cmakemake.py to make the last step to complete all of this. |
Any update on this one? |
LGTM, started a couple tests |
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 6 out of 6 (6 easyconfigs in total) |
Test report by @SebastianAchilles Overview of tested easyconfigs (in order)
Build succeeded for 4 out of 4 (4 easyconfigs in total) |
Test report by @SebastianAchilles Overview of tested easyconfigs (in order)
Build succeeded for 4 out of 4 (4 easyconfigs in total) |
easybuild/easyblocks/b/boost.py
Outdated
@@ -120,6 +123,13 @@ def prepare_step(self, *args, **kwargs): | |||
def configure_step(self): | |||
"""Configure Boost build using custom tools""" | |||
|
|||
# boost_multi_thread is deprecated | |||
if self.cfg['boost_multi_thread'] is not None: | |||
self.log.deprecated("boost_multi_thread has been deprecated, it has been replaced by tagged_layout. " |
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.
@akesandgren The easyconfigs for Boost 1.74.0 should be updated accordingly, since they set boost_multi_thread = True
.
Couldn't find a PR for that, did I overlook it?
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.
No I didn't make one yet. Didn't know if we wanted to change that already or not.
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 13 out of 13 (13 easyconfigs in total) |
…gren/easybuild-easyblocks into 20210602164441_new_pr_jgYrgPRJSF
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
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.
lgtm
Followup to easybuilders#2456 This now saves some work by building Boost.MPI with the main build and installing directly into the final folder without an intermediate copy step Also a bit of cleanup, fixing comments, ...
Followup to easybuilders#2456 This now saves some work by building Boost.MPI with the main build and installing directly into the final folder without an intermediate copy step Also a bit of cleanup, fixing comments, ...
Followup to easybuilders#2456 This now saves some work by building Boost.MPI with the main build and installing directly into the final folder without an intermediate copy step Also a bit of cleanup, fixing comments, ...
(created using
eb --new-pr
)Fixes Boost end of old problem with using CMake and Boost.Python, still need change in cmakemake.py to complete the fix.