-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Make boost minimal and composable #22303
Conversation
This is great. Look forward to seeing this merged! Thank you! |
@eugeneswalker do you have suggestions on how to quickly restore the variants of boost in all dependents? |
38c8cec
to
adaca4f
Compare
I've just added
and added is as a dependency to all dependents of boost, so it's just another constraint. This PR is ready for review now |
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. |
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 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. |
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. |
1fdb884
to
e99db69
Compare
Ok, I've renamed
Once this is merged, I could also list all maintainers of dependent packages and ping them in a comment in this PR |
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 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. |
I guess I'll start -- hey @ax3l what |
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.
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.
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.
oops meant to request changes above
@eugeneswalker: any chance you could look at the requirements on E4S packages? |
# 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) |
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.
ping @nazavode to confirm boost components used by hipsycl
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.
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
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.
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).
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.
#22770 introduces correct deps on boost
components for newer versions too, looks like hipsycl
is ready for this PR.
756495e
to
6f0957e
Compare
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: 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? |
For hpctoolkit and dyninst, we've already done the inventory of the boost
So, I'd prefer that you remove |
I'm fine with changing all the defaults for boost libraries to False Another approach I suggested in #16909 is to add soft preferences to
This says, "I can live with boost +/-mpi, but if no one else requires I've brought this up before and the answer has always been, "wait until |
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. |
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. |
@trws That was my certainly intention 2+ years ago when I wrote these. |
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! |
2256fed
to
1d51fe8
Compare
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 |
26ea166
to
cfd1715
Compare
Nice, thanks! |
@@ -35,6 +36,11 @@ class FluxSched(AutotoolsPackage): | |||
variant('cuda', default=False, description='Build dependencies with support for CUDA') | |||
|
|||
depends_on("boost+graph@1.53.0,1.59.0:") | |||
|
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.
@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.
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 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.
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.
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.
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 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("boost+graph@1.53.0,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("libxml2@2.9.1:")
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 flux-sched@0.17.0 12:20:01 ()
Input spec
--------------------------------
flux-sched@0.17.0
Concretized
--------------------------------
flux-sched@0.17.0%gcc@4.9.3~cuda arch=linux-rhel7-x86_64
^boost@1.76.0%gcc@4.9.3~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 flux-sched@0.12.0 12:20:44 ()
Input spec
--------------------------------
flux-sched@0.12.0
Concretized
--------------------------------
flux-sched@0.12.0%gcc@4.9.3~cuda arch=linux-rhel7-x86_64
^boost@1.76.0%gcc@4.9.3~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:
boost@1.76.0%gcc@4.9.3~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:
flux-sched@0.17.0%gcc@4.9.3~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?
46cef71
to
77f9486
Compare
@trws what's the status here actually? |
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$>.
|
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.
77f9486
to
6105ff6
Compare
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 onboost +component_a ~component_b
. And if packge B only needs+component_b
it might depend onboost ~component_a +component_b
. Ifpackage 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 onboost +component_b
andpackage 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.