Make boost minimal and composable by haampie · Pull Request #22303 · spack/spack

haampie · 2021-03-15T15:33:08Z

Make boost composable

Currently Boost enables a few components through variants by default,
which means that if you want to use only what you need and no more, you
have to explicitly disable these variants, leading to concretization
errors whenever a second package explicitly needs those components.

For instance if package A only needs +component_a it might depend on
boost +component_a ~component_b. And if packge B only needs
+component_b it might depend on boost ~component_a +component_b. If
package C now depends on both A and B, this leads to unsatisfiable
variants and hence a concretization error.

However, if we default to disabling all components, package A can simply
depend on boost +component_a and package B on boost +component_b and
package C will concretize to ^boost +component_a +component_b,
and whatever you install, you get the bare minimum.

I've just restored the originally enabled variants by default for 5 of
the 200+ dependents of boost, but not pursuing that until this gets
feedback.

eugeneswalker · 2021-03-16T23:29:11Z

This is great. Look forward to seeing this merged! Thank you!

haampie · 2021-03-17T19:16:49Z

@eugeneswalker do you have suggestions on how to quickly restore the variants of boost in all dependents?

haampie · 2021-03-17T23:36:17Z

I've just added

>>> Boost.sensible_default_spec
boost+atomic+chrono+date_time+exception+filesystem+graph+iostreams+locale+log+math+program_options+random+regex+serialization+signals+system+test+thread+timer+wave

and added is as a dependency to all dependents of boost, so it's just another constraint.

This PR is ready for review now

adamjstewart · 2021-03-21T14:01:12Z

I don't understand the sensible default spec stuff. That will add a hard requirement on those variants. OpenCV is a package with a similar problem of too many variants. I recently disabled all variants by default and it's been working pretty well.

haampie · 2021-03-21T15:13:12Z

It's added to ensure all dependents continue to have boost components that were enabled by default prior to this pr.

After this PR package authors can decide to drop depends_on(Boost.sensible_default_spec) and replace it with the minimal set of components they really need like for instance depends_on('boost +filesystem +regex').

I don't want to go over 200+ packages to see what components of boost they really need; let's leave that to the package authors.

adamjstewart · 2021-03-21T17:43:47Z

I see. Personally, I'm fine with breaking all of those packages and waiting for PRs to come in to fix them, but you're nicer than me lol. I'm fine with including those by default but it would be good to add a comment saying that this line probably isn't needed and someone needs to figure out which boost components are actually required.

haampie · 2021-03-21T18:15:32Z

Ok, I've renamed Boost.sensible_default_spec with Boost.with_default_variants and added a comment

    # TODO: replace this with an explicit list of components of Boost,
    # for instance depends_on('boost +filesystem')
    # See https://github.com/spack/spack/pull/22303 for reference

Once this is merged, I could also list all maintainers of dependent packages and ping them in a comment in this PR

tgamblin · 2021-03-21T18:32:49Z

I'm with @adamjstewart on this one -- can we just keep the dependencies as they were, and ping maintainers on this PR before merge? Let's just ask them what dependencies they had, and assume they did not care before. If they really did care, they were supposed to put boost +needed_thing.

I think making packages minimal will encourage this, so I really like the PR, but I don't think we should make a habit of having all the packages that depend on boost import it and add special dependencies for changes like this. Ideally we'd verify by building all of the packages (we'll get there eventually) but for now this is what we've got.

tgamblin · 2021-03-21T18:33:51Z

I guess I'll start -- hey @ax3l what boost variants does warpx actually depend on?

tgamblin

Starting to look at boost requirements. Looks like a lot of packages actually do not specify them, so I think we should definitely ping maintainers.

var/spack/repos/builtin/packages/abyss/package.py

var/spack/repos/builtin/packages/3dtk/package.py

var/spack/repos/builtin/packages/acts/package.py

var/spack/repos/builtin/packages/adept-utils/package.py

var/spack/repos/builtin/packages/adol-c/package.py

var/spack/repos/builtin/packages/akantu/package.py

var/spack/repos/builtin/packages/alps/package.py

tgamblin

oops meant to request changes above

tgamblin · 2021-03-21T18:58:12Z

@eugeneswalker: any chance you could look at the requirements on E4S packages?

var/spack/repos/builtin/packages/warpx/package.py

var/spack/repos/builtin/packages/gearshifft/package.py

ax3l · 2021-03-21T19:41:45Z

var/spack/repos/builtin/packages/hipsycl/package.py

+    # TODO: replace this with an explicit list of components of Boost,
+    # for instance depends_on('boost +filesystem')
+    # See https://github.com/spack/spack/pull/22303 for reference
+    depends_on(Boost.with_default_variants)


ping @nazavode to confirm boost components used by hipsycl

Looks like this to me: https://github.com/illuhad/hipSYCL/blob/develop/CMakeLists.txt#L35

context

fiber

and for tests: https://github.com/illuhad/hipSYCL/blob/develop/tests/CMakeLists.txt#L6

unit_test_framework

Hi @ax3l, thanks for the ping. The current version we are providing via spack depends on filesystem and preprocessor (plus test and mpl when unit tests are enabled, something we are not currently exposing to users). context and fiber will be needed with the shiny brand new version of hipsycl (something I'm looking into right now).

#22770 introduces correct deps on boost components for newer versions too, looks like hipsycl is ready for this PR.

var/spack/repos/builtin/packages/isaac/package.py

var/spack/repos/builtin/packages/isaac-server/package.py

var/spack/repos/builtin/packages/nalu-wind/package.py

var/spack/repos/builtin/packages/warpx/package.py

trws · 2021-07-21T16:18:23Z

We've been doing some work on the side for this as part of the BUILD project. Using boost, and this branch, as a build suite we got some data on all the boost components touched by a c++ compiler as part of building each of the packages depending on boost (at least the ones that actually built). As a result, we have a full list of boost components used by about half the packages touched in here so we can replace the defaults. Thanks to @ryandomalley for compiling the data, we have a listing of everything here:
Boost.md

I'm going to create an issue with all of these listed out so we can track progress and get everything updated. Will link back here when everything is ready. @haampie, are you good with having PRs target this PR's branch so we can merge everything together and get this in?

mwkrentel · 2021-07-21T18:44:14Z

For hpctoolkit and dyninst, we've already done the inventory of the boost
libraries that we need and have explicitly required them since day 1.

boost_libs = (
    '+atomic +chrono +date_time +filesystem +system +thread +timer'
    ' +graph +regex +shared +multithreaded visibility=global'
)
depends_on('boost' + boost_libs)

So, I'd prefer that you remove depends_on(Boost.with_default_variants) for
these two packages. It's not necessary and we're already doing what you suggest.

@hainest

mwkrentel · 2021-07-21T18:50:50Z

I'm fine with changing all the defaults for boost libraries to False
and then turning on only those things that are required. That's an Ok
way of handling the "can we all get along" problem with multiple
packages.

Another approach I suggested in #16909 is to add soft preferences to
depends in package.py.

depends_on('boost', prefer='~mpi')

This says, "I can live with boost +/-mpi, but if no one else requires
anything, then I'd prefer False (because I don't use it and don't want
to build it)."

I've brought this up before and the answer has always been, "wait until
the new concretizer and then it will be easier to do."

trws · 2021-07-21T19:03:28Z

I agree it would be nice to have soft preferences, for this specific issue @ryandomalley is looking at helping us actually break up boost into individual packages though, which would be even better. For now though this is a good first step.

@mwkrentel Are you sure that list is all of the features dyninst will need including those that are no longer built by default? There are more variants to cover the entries that didn't exist before, that's why I did it the way I did. If the list is already exhaustive though I'll be happy to take it and remove the default list.

trws · 2021-07-21T19:04:32Z

Oh, and @mwkrentel, look here for the current version. I just went through and did a big merge and fix that touched the dyninst package among others to try to make use of the existing lists.

mwkrentel · 2021-07-21T19:48:51Z

Are you sure that list is all of the features dyninst will need
including those that are no longer built by default?

@trws That was my certainly intention 2+ years ago when I wrote these.
But software rot is possible, so I'll check with my group (hpctoolkit)
and dyninst. Assume everything is Ok unless I get back to you.

haampie · 2021-07-21T21:25:27Z

Very cool @trws!

I think something went wrong with the commit you've pushed though, I think you can (force) push to the branch directly, feel free to do so!

haampie · 2021-07-21T22:00:17Z

I've rebased this branch onto develop to get rid of the conflicts. The previous state of the PR is here: https://github.com/haampie/spack/tree/feature/make-boost-composable-old

trws · 2021-07-21T22:35:28Z

Nice, thanks!

SteVwonder · 2021-07-21T22:48:52Z

var/spack/repos/builtin/packages/flux-sched/package.py

    variant('cuda', default=False, description='Build dependencies with support for CUDA')

    depends_on("[email protected],1.59.0:")
+


@trws : happy to help, but I think I need some clarification on what you'd like me to test. We have the +graph variant defined above. I think the package has been like that for a while. That should be sufficient, right?

Actually, now that I'm thinking about it, the new data-staging plugin requires filesystem, so that's one change that needs to be made. But besides that, I'm a little lost as to what behavioral changes this PR makes and what needs to be tested on the Flux-side as a consequence.

The boost package will no longer have a default set of stuff that gets built in addition to the selected variants, so anything that gets used but isn't listed because its variant didn't exist will cause issues. Basically, if it builds with boost+graph+filesystem after this PR, it's all good, but it might not if something that used to be included by default is no longer there.

Makes sense. Sorry, I just now am seeing the context at the top of this PR. I just saw your issue that I was tagged in and went straight to this diff. My bad.

I'll definitely add it to my TODO list. No promises on a timeline. Probably will get around to it shortly after the next monthly Flux release when I drop the next set of release numbers and checksum.

I applied this patch to the current PR branch:

diff --git a/var/spack/repos/builtin/packages/flux-sched/package.py b/var/spack/repos/builtin/packages/flux-sched/package.py index 41781aa232..e2bbff501a 100644 --- a/var/spack/repos/builtin/packages/flux-sched/package.py +++ b/var/spack/repos/builtin/packages/flux-sched/package.py @@ -36,12 +36,10 @@ class FluxSched(AutotoolsPackage): variant('cuda', default=False, description='Build dependencies with support for CUDA') depends_on("[email protected],1.59.0:") + depends_on("boost+filesystem", when="@0.13.0:") - # TODO: replace this with an explicit list of components of Boost, - # for instance depends_on('boost +filesystem') - # See https://github.com/spack/spack/pull/22303 for reference - depends_on(Boost.with_default_variants) depends_on("py-pyyaml") + depends_on("py-jsonschema", when="@0.8.0:") depends_on("[email protected]:") depends_on("yaml-cpp") depends_on("uuid")

I can concretize flux-sched with the proper boost variants when I provide an explicit version for flux-sched:

herbein1@quartz1922 ~/Repositories/spack (boost !?) ❯ spack spec [email protected] 12:20:01 () Input spec -------------------------------- [email protected] Concretized -------------------------------- [email protected]%[email protected]~cuda arch=linux-rhel7-x86_64 ^[email protected]%[email protected]~atomic~chrono~clanglibcpp~container~context~coroutine~date_time~debug~exception~fiber+fi lesystem+graph~icu~iostreams~locale~log~math~mpi+multithreaded~numpy~pic~program_options~python~random~regex~seriali zation+shared~signals~singlethreaded~system~taggedlayout~test~thread~timer~versionedlayout~wave cxxstd=98 visibility =hidden arch=linux-rhel7-x86_64 <snip>

It also works for older flux-sched's that don't require boost+filesystem:

herbein1@quartz1922 ~/Repositories/spack (boost !?) ❯ spack spec [email protected] 12:20:44 () Input spec -------------------------------- [email protected] Concretized -------------------------------- [email protected]%[email protected]~cuda arch=linux-rhel7-x86_64 ^[email protected]%[email protected]~atomic~chrono~clanglibcpp~container~context~coroutine~date_time~debug~exception~fiber~fi lesystem+graph~icu~iostreams~locale~log~math~mpi+multithreaded~numpy~pic~program_options~python~random~regex~seriali zation+shared~signals~singlethreaded~system~taggedlayout~test~thread~timer~versionedlayout~wave cxxstd=98 visibility =hidden arch=linux-rhel7-x86_64

But if you don't provide an explicit version for flux-sched, everything borks:

9s herbein1@quartz1922 ~/Repositories/spack (boost !?) ❯ spack spec flux-sched 12:20:30 () Input spec -------------------------------- flux-sched Concretized -------------------------------- ==> Error: An unsatisfiable variant constraint has been detected for spec: [email protected]%[email protected]~atomic~chrono~clanglibcpp~container~context~coroutine~date_time~debug~exception~fiber~fil esystem+graph~icu~iostreams~locale~log~math~mpi+multithreaded~numpy~pic~program_options~python~random~regex~serializ ation+shared~signals~singlethreaded~system~taggedlayout~test~thread~timer~versionedlayout~wave cxxstd=98 visibility= hidden arch=linux-rhel7-x86_64 while trying to concretize the partial spec: [email protected]%[email protected]~cuda arch=linux-rhel7-x86_64 flux-sched requires boost variant +filesystem, but spec asked for ~filesystem

Not sure why it is saying that the spec is asking for ~filesystem. Any thoughts/advice?

haampie · 2021-10-01T11:55:33Z

@trws what's the status here actually?

trws · 2021-10-01T19:30:51Z

At the moment, we need to get packages updated/tested to use the detected dependencies so we can actually merge this in. I'm actually talking with a new team member about doing some of this work starting later today, so hopefully we'll start making more progress soon.

…

________________________________ From: Harmen Stoppels ***@***.***> Sent: Friday, October 1, 2021 4:55 AM To: spack/spack ***@***.***> Cc: Scogland, Tom ***@***.***>; Mention ***@***.***> Subject: Re: [spack/spack] Make boost minimal and composable (#22303) @trws<https://urldefense.us/v3/__https://github.com/trws__;!!G2kpM7uM-TzIFchu!ifz6ZAP5BRSnOgkBxyw9sBz4ow1L1KUhN8hiOKJUoJ8dRY9CZ0OpDe1abLJjpOGeYQ$> what's the status here actually? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://urldefense.us/v3/__https://github.com/spack/spack/pull/22303*issuecomment-932163163__;Iw!!G2kpM7uM-TzIFchu!ifz6ZAP5BRSnOgkBxyw9sBz4ow1L1KUhN8hiOKJUoJ8dRY9CZ0OpDe1abLLjTMqaDg$>, or unsubscribe<https://urldefense.us/v3/__https://github.com/notifications/unsubscribe-auth/AAFBFNJ7YYBWRD7B323ZCDDUEWOUBANCNFSM4ZGY6TPQ__;!!G2kpM7uM-TzIFchu!ifz6ZAP5BRSnOgkBxyw9sBz4ow1L1KUhN8hiOKJUoJ8dRY9CZ0OpDe1abLJ34SYx3A$>.

haampie · 2021-10-01T20:02:03Z

Cool, thanks! :)

Currently Boost enables a few components through variants by default, which means that if you want to use only what you need and no more, you have to explicitly disable these variants, leading to concretization errors whenever a second package explicitly needs those components. For instance if package A only needs `+component_a` it might depend on `boost +component_a ~component_b`. And if packge B only needs `+component_b` it might depend on `boost ~component_a +component_b`. If package C now depends on both A and B, this leads to unsatisfiable variants and hence a concretization error. However, if we default to disabling all components, package A can simply depend on `boost +component_a` and package B on `boost +component_b` and package C will concretize to depending on `boost +component_a +component_b`, and whatever you install, you get the bare minimum.

tldahlgren added bootstrap Anything that has to do with Spack building its own dependencies. variants dependencies and removed bootstrap Anything that has to do with Spack building its own dependencies. labels Mar 15, 2021

haampie force-pushed the feature/make-boost-composable branch from 38c8cec to adaca4f Compare March 17, 2021 23:33

haampie requested a review from adamjstewart March 17, 2021 23:37

haampie closed this Mar 18, 2021

haampie reopened this Mar 18, 2021

haampie closed this Mar 21, 2021

haampie reopened this Mar 21, 2021

haampie force-pushed the feature/make-boost-composable branch from 1fdb884 to e99db69 Compare March 21, 2021 18:13

tgamblin approved these changes Mar 21, 2021

View reviewed changes

tgamblin requested changes Mar 21, 2021

View reviewed changes