Add a new `drop` directive for extending packages by tgamblin · Pull Request #35045 · spack/spack

tgamblin · 2023-01-19T23:06:51Z

Closes #33506; related to #33057.

When you inherit from a package in Spack, you inherit all the metadata from its directives, including version, provides, depends_on, conflicts, etc.

In some cases, you may not want this metadata. This PR adds a new drop directive that allows you to throw out things inherited from the base class. For example:

from spack.pkg.builtin.mpich import Mpich

class MyMpich(Mpich):
    drop("versions")    # don't inherit any versions from builtin Mpich
    version("5.0", "08721a102fefcea2ae4add8c9cc548df77e9224f5385ad0872a9150fdd26a415")
    version("6.0", "9cc39dd33dd4227bb82301d285437588d705290846d22ab6b8791c7e631ce385")

Without the drop("versions") directive, this package would have versions 5.0, 6.0, and anything inherited from Mpich. With it, this package has only versions 5.0 and 6.0.

You can drop any of: conflicts, dependencies, extendees, patches, provided, resources, variants, or versions.

add new drop directive
Two packages were modifying their versions dictionary in their constructors to achieve this, but this causes spack url stats to fail with a concurrent modification exception as it iterates over all packages. Fixed these to use drop instead.
Update documentation
Add tests

When you inherit from a package in Spack, you inherit all the metadata from its directives, including `version`, `provides`, `depends_on`, `conflicts`, etc. In some cases, you may not want this metadata. This PR adds a new `disinherit` directive that allows you to throw out things inherited from the base class. For example: ```python from spack.pkg.builtin.mpich import Mpich class MyMpich(Mpich): disinherit("versions") # don't inherit any versions from builtin Mpich version("5.0", "08721a102fefcea2ae4add8c9cc548df77e9224f5385ad0872a9150fdd26a415") version("6.0", "9cc39dd33dd4227bb82301d285437588d705290846d22ab6b8791c7e631ce385") ``` Without the `disinherit("versions")` directive, this package would have versions `5.0`, `6.0`, *and* anything inherited from `Mpich`. With it, this package has only versions `5.0` and `6.0`. You can `disinherit` any of: `conflicts`, `dependencies`, `extendees`, `patches`, `provided`, `resources`, `variants`, or `versions`. - [x] add new `disinherit directive` - [x] Two packages were modifying their `versions` dictionary in their constructors to achieve this, but this causes `spack url stats` to fail with a concurrent modification exception as it iterates over all packages. Fixed these to use `disinherit` instead. - [x] Update documentation - [x] Add tests

tgamblin · 2023-01-19T23:15:33Z

Would like to get some thoughts on this before it gets merged, so don't just merge it after one approval :)

w8jcik · 2023-01-20T10:19:14Z

The change looks great. Thank you for the effort that you put into adding it.

Coming to implementation, disinherit could accept predicate what to remove, but it would be also unnecessary complication. Current solution covers my use case and looks simple.

So again, it looks good to me :)

alalazo

I think this works very well for simple packages.

The current implementation though require users to duplicate code, if they want to get rid of e.g. a single variant. This is fine for small packages, less so if we want to remove a single variant, or conflict, from a package like trilinos.

Another consideration is that methods inherited from base classes might refer to entities that got pruned, and produce some inconsistent behavior if people don't carefully check the recipe.

lib/spack/spack/directives.py

tgamblin · 2023-01-20T17:53:09Z

The current implementation though require users to duplicate code, if they want to get rid of e.g. a single variant.

I was thinking of adding:

disinherit("versions", except=["1.0", "2.0", ...])

and, e.g.:

# keeps all provides("mpi...") (by matching the spec)
disinherit("provided", except="mpi")

and:

# get rid of any conflicts with hdf5 > 10
disinherit("conflicts", except="hdf5@:10")

Of the things you could disinherit:

Versions have obvious identifiers (the version) and also have satisfies(), e.g. disinherit("versions", except="10:")
Variants and resources have names
Conflicts, dependencies, extendees, and provided can be matched with spec semantics
Patches I don't really know what to do with -- you could specify the sha256, the URL, or the path but they all seem cumbersome. I would lean towards just disallowing except= for patches unless we really need this.

If that sounds good it wouldn't be hard to throw something together.

becker33 · 2023-01-20T18:29:47Z

This looks substantially like a duplicate of #33506

Patches I don't really know what to do with -- you could specify the sha256, the URL, or the path but they all seem cumbersome. I would lean towards just disallowing except= for patches unless we really need this.

We definitely need to be able to disinherit patches, but I don't off the top of my head have a use-case for except

tgamblin · 2023-01-20T19:49:00Z

@becker33: Thanks -- I guess I missed #33506. Sorry @iarspider -- I didn't mean to step on your toes.

Personally I like this better b/c it's a single directive, not a bunch of them, and I think we need to be able to drop all of something and also drop specific things with except=. I kind of like drop() from #33506 instead of disinherit() -- would that alleviate your concern @alalazo?

I would be curious what @iarspider thinks of this -- with the changes mentioned in #35045 (comment) would this work for you?

I think I can bring over the filename/url naming for patches from #33506 and adapt @iarspider's test too.

iarspider · 2023-01-20T22:18:32Z

@tgamblin I would prefer the opposite syntax - disinherit(..., only=...), since I rarely need to drop more than one of something. Or maybe both, while obv not allowing things like disinherit(..., only=..., except=...) :)

tgamblin · 2023-01-20T22:40:50Z

@iarspider: We were thinking we'd go with your drop() over disinherit -- is that ok?

I don't mind doing both drop(..., only="") and drop(..., except="")... I suspect someone will ask for both eventually.

iarspider · 2023-01-20T22:42:36Z

Sure. I don't care about the actual name.

…

On Fri, Jan 20, 2023, 23:41 Todd Gamblin ***@***.***> wrote: @iarspider <https://github.com/iarspider>: We were thinking we'd go with your drop() over disinherit -- is that ok? I don't mind doing both drop(..., only="") and drop(..., except="")... I suspect someone will ask for both eventually. — Reply to this email directly, view it on GitHub <#35045 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE3NOQ46MHXHQFMOXRVTTLWTMH73ANCNFSM6AAAAAAUA453WU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

becker33 · 2023-03-13T18:56:44Z

@tgamblin here is the use case for this that we discussed on our call earlier:

On RHEL systems, RHEL provides special [email protected] versions that include substantial work to make them binary compatible with the system gcc.

We want to be able to install e.g. [email protected] from a tarball of binaries to take advantage of this work. This requires being able to drop all dependencies (because they are irrelevant) and patches (they won't apply) for that one particular version (eventually probably a list of versions, to include [email protected] as well).

The ideal interface for this use case would probably be something like:

with when("@10.3.1"):
    drop("patches")
    drop("dependencies")

scheibelp · 2023-03-15T00:08:19Z

The use case that originally drew my interest to this PR does not require when: if there is a package in the builtin repository like:

class Foo(Package):
    depends_on(cmake@:upper.version)
    ...
    # a bunch of useful logic

I want to inherit Foo, but my DAG (i.e. some higher dependent) needs a CMake version after upper.version
I experiment personally with installing Foo using that later CMake and find that it works

I can submit a PR to loosen the constraints in Foo but:

To use that I pull the latest state of builtin (along with possibly other incompatible changes to other packages)
While waiting I need to maintain this change as a patch to the Foo package (e.g. a .patch file, or a separate branch of Spack with those changes applied)

So it would be nice to inherit Foo but discard the constraints that aren't necessary:

class Foo(builtin.Foo):
    drop("dependencies")

ideally, we could drop only specific dependencies and keep a subset of them, but for this use case it shouldn't be a major issue to rewrite the depends_on constraints that I want to keep.

alalazo · 2023-03-17T21:46:27Z

I have a couple of use cases where drop would be useful. One is build system specific:

cmake build system: make "generator" a variant #35552 (review)

In that case I am using conflicts to hide some weird implementation details when we have to drop one of the two generators. A better implementation would be using drop to drop the generator variant, and any metadata dependent on the presence of the value that isn't used.

The other one is a from a user (can't find the PR right now). That user wanted to override for a package the list of supported cuda_arch, but needs to get rid to all the metadata that reference them (which is inherited from the base class).

danielsjensen1 · 2025-02-08T18:14:11Z

I think this works very well for simple packages.

The current implementation though require users to duplicate code, if they want to get rid of e.g. a single variant. This is fine for small packages, less so if we want to remove a single variant, or conflict, from a package like trilinos.

Another consideration is that methods inherited from base classes might refer to entities that got pruned, and produce some inconsistent behavior if people don't carefully check the recipe.

I put in an implementation that allows for the removal of individual directives in addition to entire categories at #48947 as that has primarily been my use case. I often need to remove just one conflict when I'm inheriting a package. I didn't see this issue earlier so I was dumb and wrote a completely separate implementation.

github-actions · 2025-08-10T01:53:45Z

This pull request has been automatically marked as stale because it has not had
any activity in the last 6 months. It will be closed if there is no further activity.
Thank you for your contributions!

github-actions · 2025-09-10T01:50:29Z

This pull request was closed because it had no activity for 30 days after being marked stale. If you feel this is in error, please feel free to reopen this pull request.

becker33

There are two levels of additions to this that I think are necessary, but I don't know that they need to be part of this PR.

The use case to tighten a when constraint:

If we have a builtin package foo with some dependencies

class Foo(Package):
    depends_on("bar", when="+extra_capabilities")
    ... # 5000 lines of additional dependencies

and a user discovers that bar is not actually necessary for +extra_capabilities after some version cut-off X.Y.Z. Then the user will want to drop the specific dependency on "bar" without having to recreate the rest of the dependencies, so that they can recreate the constrained bar dependencies.

from spack_repo.builtin.packages.foo.package import Foo as BuiltinFoo
class Foo(BuiltinFoo):
    drop("dependencies", "bar")
    depends_on("bar", when="@:X.Y +extra_capabilities")

The further case that we've discussed more, regarding when clauses on the drop directive

I'm fully convinced that (2) should wait for a follow-on PR (if we determine it's necessary. But (1) is much more essential to the use case in my mind. It seems we would want to be able to drop:

A particular version by its version string
A conflict by its first parameter (arbitrarily, this one is less clear how it maps to user intuition than the others
A dependency by either its name or by a matching spec (not sure which is better)
An extension by the same as a dependency
A provider by the same as a dependency (more likely name in this case)
A patch by its name/url
A variant by its name
A resource by its name/url
(no need for dropping a specific build_system, since it's implemented as a variant)

All of these seem reasonably easy to match on if we can settle on a clear interface and documentation of what the additional argument does in terms of constraining the drop.

The obvious next step of conditions/exceptions to the constraint is clearly a future work issue.

becker33 · 2025-09-17T10:33:14Z

lib/spack/docs/packaging_guide.rst

+
+.. code-block:: python
+
+   from spack.pkg.builtin.mpich import Mpich


Suggested change

from spack.pkg.builtin.mpich import Mpich

from spack_repo.builtin.packages.mpich.package import Mpich

becker33 · 2025-09-17T10:33:43Z

lib/spack/docs/packaging_guide.rst

+
+.. code-block:: python
+
+   from spack.pkg.myrepo.my_package import MyPackage


Suggested change

from spack.pkg.myrepo.my_package import MyPackage

from spack_repo.myrepo.packages.my_package.package import MyPackage

becker33 · 2025-09-17T10:34:04Z

lib/spack/docs/packaging_guide.rst

+       version("1.0", "0209444070d9c8af9b62c94095a217e3bc6843692d1e3fdc1ff5371e03aac47c")
+       version("2.0", "5dda192154047d6296ba14a4ab2d869c6926fd7f44dce8ce94f63aae2e359c5b")
+
+Every repository registered with Spack ends up in a submodule of ``spack.pkg`` with a


Suggested change

Every repository registered with Spack ends up in a submodule of ``spack.pkg`` with a

Every repository registered with Spack ends up in a submodule of ``spack_repo`` with a

becker33 · 2025-09-17T10:36:12Z

lib/spack/docs/packaging_guide.rst

+^^^^^^^^^^^^^^^^^^^^^^^^
+``disinherit``
+^^^^^^^^^^^^^^^^^^^^^^^^
+
+When you inherit from a package in Spack, you inherit all the metadata from its
+directives, including ``version``, ``provides``, ``depends_on``, ``conflicts``, etc. For
+example, ``NewPackage`` above will have four versions: ``1.0`` and ``2.0`` inherited
+from ``MyPackage``, as well as, ``3.0``, and ``4.0`` defined in ``NewPackage``.
+
+If you do not want your package to define all the same things as its base class, you can
+use the ``disinherit`` directive to start fresh in your subclass:
+
+.. code-block:: python
+
+   from spack.pkg.myrepo.my_package import MyPackage
+
+   class NewerPackage(MyPackage):
+       disinherit("versions")    # don't inherit any versions from MyPackage
+       version("5.0", "08721a102fefcea2ae4add8c9cc548df77e9224f5385ad0872a9150fdd26a415")
+       version("6.0", "9cc39dd33dd4227bb82301d285437588d705290846d22ab6b8791c7e631ce385")
+
+Now, ``NewerPackage`` will have **only** versions ``5.0`` and ``6.0``, and will not
+inherit ``1.0`` or ``2.0`` from ``MyPackage``. You can ``disinherit`` many different
+properties from base packages. The full list of options is:
+
+* ``conflicts``
+* ``dependencies``
+* ``extendees``
+* ``patches``
+* ``provided``
+* ``resources``
+* ``variants``
+* ``versions``


This is out of date in the directive name.

This talks about the composition of the versions as if no versions come from mpich.

We should discuss an example where you only want to remove one version -- in that case, you have to remove them all and reimplement the ones you want to remove

becker33 · 2025-09-17T10:38:59Z

lib/spack/spack/directives.py



+@directive()
+def disinherit(dict_name: str):


We agreed to change this to drop

becker33 · 2025-09-17T10:41:09Z

lib/spack/spack/test/directives.py

+    assert "4.0" not in pkg.versions
+
+
+def test_disinherit(config, mock_packages):


Suggested change

def test_disinherit(config, mock_packages):

def test_disinherit_invalid(config, mock_packages):

becker33 · 2025-09-17T10:41:51Z

var/spack/repos/builtin.mock/packages/disinherit/package.py

+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack.package import *
+from spack.pkg.builtin.mock.a import A


Suggested change

from spack.pkg.builtin.mock.a import A

from spack_repo.builtin_mock.packages.a.package import A

github-actions · 2026-03-17T01:52:16Z

This pull request has been automatically marked as stale because it has not had any activity in the last 6 months. It will be closed in 30 days if there is no further activity.

If the pull request is waiting for a reply from reviewers, feel free to ping them as a reminder. If it is waiting and has no assigned reviewer, feel free to ping @spack/spack-releasers or simply leave a comment saying this should not be marked stale. This will reset the pull request's stale state.

To get more eyes on your pull request, you can post a link in the #pull-requests channel of the Spack Slack.
Thank you for your contributions!

tgamblin · 2026-03-17T20:37:24Z

This is likely to be superseded by #48947 for v1.2. Closing in anticipation of that; we can reopen if it becomes interesting again.

spackbot-app bot added core PR affects Spack core functionality directives documentation Improvements or additions to documentation new-version tests General test capability(ies) update-package labels Jan 19, 2023

spackbot-app bot requested a review from w8jcik January 19, 2023 23:07

tgamblin requested review from alalazo, becker33, haampie and scheibelp January 19, 2023 23:07

tgamblin marked this pull request as draft January 19, 2023 23:15

tgamblin marked this pull request as ready for review January 20, 2023 04:22

tgamblin assigned tldahlgren and unassigned tldahlgren Jan 20, 2023

tgamblin requested a review from tldahlgren January 20, 2023 04:50

alalazo reviewed Jan 20, 2023

View reviewed changes

lib/spack/spack/directives.py Show resolved Hide resolved

tgamblin changed the title ~~Add a new disinherit directive for extending packages~~ Add a new drop directive for extending packages Jan 20, 2023

This was referenced Feb 17, 2023

cmake build system: make "generator" a variant #35552

Merged

Add directives for removing patches, conflicts and dependencies #33506

Closed

alalazo mentioned this pull request May 29, 2023

Is it possible to drop a dependency from a package using the spec syntax? #37906

Open

1 task

alalazo mentioned this pull request Sep 11, 2023

Add new package: SeLaLib #39847

Merged

tgamblin mentioned this pull request Oct 5, 2023

Unify directive dict structure #40326

Merged

tgamblin mentioned this pull request Apr 2, 2024

Add new redistribute() directive #20185

Merged

6 tasks

haampie mentioned this pull request Feb 8, 2025

Override package directives #48947

Open

github-actions bot added the stale label Aug 10, 2025

github-actions bot closed this Sep 10, 2025

becker33 reopened this Sep 17, 2025

becker33 requested changes Sep 17, 2025

View reviewed changes

github-actions bot removed the stale label Sep 18, 2025

lgarrison mentioned this pull request Oct 12, 2025

astropy: correct numpy dependency version spack/spack-packages#1952

Merged

github-actions bot added the stale label Mar 17, 2026

tgamblin closed this Mar 17, 2026


		.. code-block:: python

		from spack.pkg.builtin.mpich import Mpich

	from spack.pkg.builtin.mpich import Mpich
	from spack_repo.builtin.packages.mpich.package import Mpich


		.. code-block:: python

		from spack.pkg.myrepo.my_package import MyPackage

	from spack.pkg.myrepo.my_package import MyPackage
	from spack_repo.myrepo.packages.my_package.package import MyPackage

	Every repository registered with Spack ends up in a submodule of ``spack.pkg`` with a
	Every repository registered with Spack ends up in a submodule of ``spack_repo`` with a

		assert "4.0" not in pkg.versions


		def test_disinherit(config, mock_packages):

	def test_disinherit(config, mock_packages):
	def test_disinherit_invalid(config, mock_packages):

	from spack.pkg.builtin.mock.a import A
	from spack_repo.builtin_mock.packages.a.package import A

Conversation

tgamblin commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Jan 19, 2023

Uh oh!

w8jcik commented Jan 20, 2023

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgamblin commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

becker33 commented Jan 20, 2023

Uh oh!

tgamblin commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iarspider commented Jan 20, 2023

Uh oh!

tgamblin commented Jan 20, 2023

Uh oh!

iarspider commented Jan 20, 2023 via email

Uh oh!

becker33 commented Mar 13, 2023

Uh oh!

scheibelp commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Mar 17, 2023

Uh oh!

danielsjensen1 commented Feb 8, 2025

Uh oh!

github-actions bot commented Aug 10, 2025

Uh oh!

github-actions bot commented Sep 10, 2025

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

becker33 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

becker33 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

becker33 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

becker33 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

becker33 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

becker33 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

becker33 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 17, 2026

Uh oh!

tgamblin commented Mar 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tgamblin commented Jan 19, 2023 •

edited

Loading

tgamblin commented Jan 20, 2023 •

edited

Loading

tgamblin commented Jan 20, 2023 •

edited

Loading

scheibelp commented Mar 15, 2023 •

edited

Loading