@haampie I'm not competent to speak to the technical aspects of the implementation, but as you describe it I think this should do what we need, thank you!

I've had chance to try this out and I've obtained encouraging results but they're not 100%.

Procedure:

1. Install an environment from spack.yaml file on machine A, write results (sans roots) to BuildCache.
2. On machine B (with a nominally compatible packages.yaml) file, update Spack installation with respect to upstream develop and apply this PR as a rebased cherry-pick.
3. With the BuildCaches written in step 1 configured as mirrors, attempt to install an environment from the same spack.yaml file.
4. Multiple packages get recompiled (aside from the non-cached roots):

   ==> [2023-03-22-16:39:24.268978] No binary for perl-io-compress-2.204-unvgym26d4tyiusddjd27f6z7a7kqswv found: installing from source
   ==> [2023-03-22-16:39:41.576184] No binary for perl-devel-symdump-2.18-kzg4qdyndciston3rftikkhgfnu7okym found: installing from source
   ==> [2023-03-22-16:39:51.889217] No binary for libarchive-3.6.2-h3djzt3zlnxh6frkw5cwvgmbn4x4s2og found: installing from source
   ==> [2023-03-22-16:41:22.143673] No binary for perl-json-4.10-f7lvwdc3ipwoucgiif6ggvgiwtrrj2ru found: installing from source
   ==> [2023-03-22-16:41:30.668832] No binary for cmake-3.25.2-dlvgjd6zc6kkydwvp7l7c62g7th2esrc found: installing from source
   ==> [2023-03-22-16:43:54.562845] No binary for py-pybind11-2.10.1-bhcnacgz3mifdmvxcv7zotvpjia5v7ea found: installing from source
   ==> [2023-03-22-16:44:06.168718] No binary for xrootd-5.5.3-wqss46lvtj3nm4wkl2rmgvmg2ic7fm27 found: installing from source
   ==> [2023-03-22-16:45:31.339243] No binary for root-6.28.00-mv762opqomkhq3a63wlk5xgjneprenzb found: installing from source
   ==> [2023-03-22-17:13:20.793309] No binary for perl-mce-1.884-e3nqgfxzlyrkwdrnra22zjltkpz27uqo found: installing from source
   ==> [2023-03-22-17:13:33.407071] No binary for perl-perl-critic-dynamic-0.05-2mgx7nv2vcetqwkw3xlve2ngfho6oojt found: installing from source
   ==> [2023-03-22-17:13:36.822645] No binary for perl-test-perl-critic-1.04-phhbx5aq4xpcwehhqcxqzqxln4lv4noe found: installing from source
   ==> [2023-03-22-17:13:43.923441] No binary for perl-perl-critic-bangs-1.12-yz5kd3xdu7ikh3wn4sakz6n6z4d5hep5 found: installing from source
   ==> [2023-03-22-17:13:46.878057] No binary for perl-perl-critic-swift-1.0.3-g3reemqpwte67q74zvmiahoz2nzvmlx2 found: installing from source
   ==> [2023-03-22-17:13:50.615230] No binary for perl-task-perl-critic-1.008-kxftpdv77beqqmtbnzxtoffmunmnrr43 found: installing from source
   

I should note that an environment created from spack-A.lock on machine B installs everything possible from cache, although that is also true without this PR.

I include within the attached spack-files.tgz:

• critic@[email protected]@4.8.5.yaml
• packages-A.yaml
• spack-A.lock
• packages-B.yaml
• spack-B.lock

If there were some way for me to trace the source of the divergence myself, please let me know and I would be happy to do the work of ascertaining whether this is PEBKAC, GIGO, or a limitation of this PR.

@haampie What's the status on this, please?

@haampie Bump?

@haampie What is necessary for this to be made ready to merge, please, and can I help?

@haampie @scheibelp @tgamblin Ping?

Hi @greenc-FNAL, this isn't moving forward cause other people have different preferences.

@alalazo wants to avoid the intermediate step of externals in packages.yaml

@becker33 wants to solve this as a special case of splicing (although not 100% sure if your use case was discussed explicitly in that context)

In my opinion:

• I don't see packages.yaml external config go away: (1) not every package can be auto-detected, and (2) sharing an env with externals requires externals in config, (3) directly interacting with the db is poor user experience, (4) config is scoped while the db is global

• Splicing as a solution to your particular issue is a bit of a sledgehammer, it's a pretty trivial problem so why not a trivial solution like this PR.

Since currently externals use abstract specs, and spack fills out the details in some way the user can't control, I don't see why filling out the blanks from a matching spec in a build cache is controversial.

Thoughts @becker33 / @alalazo / @tgamblin ?

I don't see packages.yaml external config go away: (1) not every package can be auto-detected, and (2) sharing an env with externals requires externals in config, (3) directly interacting with the db is poor user experience, (4) config is scoped while the db is global

There's a fundamental misunderstanding here on what I want. I don't want packages.yaml to go away, it will still stay for any user defined spec. What I want is for a DB¹ to be the source of truth, and for anything that is auto-detected to not appear in the packages.yaml. The main point for doing that is not to concretize to arbitrary values the variant which we don't detect, and to treat externals like any other "reused" spec. This deals with (1) and (2).

For (3), that issue means to me that we should make it easier to deal with the DB. The DB contains anything that was concretized and is frozen, and in my opinion we shouldn't be able to change it from configuration. What we should be able to do is to update the DB on system updates, to reflect the current state. Hashes should change accordingly when we splice some new external spec because e.g. it bumped version.

In #35975 (comment) and in #40843 (comment) it seems you suggest we should be less strict with externals, and allow people to fix the behavior of externals by editing a configuration file. How would you deal with that, and with the fact that hashes are frozen once something is installed?

Regarding #35975 (comment), I'm not sure why auto-detected system packages should not appear in packages.yaml:

• I assume there is agreement that the flexibility of manually-added externals is important: if a missing variant does not satisfy a requested variant, then I anticipate that externals would be largely ignored (or for buildable:false would require a process of refinement that is burdensome)
• Likewise, I think the same case can be made for autodetected specs (although possibly not as strongly, since spack external find doesn't mandate the use of auto-detected packages unless --not-buildable is specified)

I assume there is agreement that the flexibility of manually-added externals is important

Sure, but in the end an external has to be registered in the DB when a spec is concretized and installed, right? If that is the case, what does:

(3) directly interacting with the db is poor user experience,

mean (see also discussion in #40843 (comment))? If it means that we should make external specification more loose, and allow changing a config file to also change the behavior of something that in the DB, then imo it's the wrong path - we would give less meaning to the hash in that way.

As an example: the issue linked to this PR deals with adding / removing modules to an already installed external. What if the typo was in the version? Should we just proceed with editing the config file without modifications to the DB to fix that?

@scheibelp

I'm not sure why auto-detected system packages should not appear in packages.yaml:

This is another discussion, and a bit off-topic in this PR (so maybe it's better to discuss details in #36179). Briefly, the main reason is that a DB should be the ultimate source of truth for concrete specs, and externals are concrete specs.

packages.yaml is currently an intermediate step, which is more user friendly if things are specified manually. If Spack is detecting the externals, there is no need for this intermediate step. That:

1. Allows avoiding adding arbitrary variants which were not detected
2. Allows to deal with externals in the same way we deal with other concrete specs (through "reuse")
3. Allows extending external detection to specs with dependencies (since a DB can already store them)

@alalazo can we park this packages.yaml discussion? I disagree with many points, but since you're saying you don't want the config to go away, it shouldn't be a blocker for this PR.

it shouldn't be a blocker for this PR.

I think we need to decide what we want as a path forward for externals in general (let's discuss this tomorrow during the meeting?) If we want to move to externals in the DB like we discussed, then we also need to be clear how the semantic in
_is_reusable_external will be changed later to avoid backward incompatibilities.

Can you list possible future conflicts of this change? I don't see any.

Here you match on abstract specs in packages.yaml If later on those specs are considered concrete, and stored as such, we either need to:

1. Redefine what satisfies mean for concrete external specs (where concrete foo+bar and concrete foo~bar both satisfy concrete foo)
2. Introduce backward incompatibility with the semantic added by this PR

Can you answer the questions above or in the issue #40843 (comment) how do you want to achieve that:

any config update to modules takes effect immediately.

(which I guess is to be extended to other aspects of the spec) ?

This should address your comments:

Externals config as a filter on reusable externals from build caches as introduced in this PR can be extended to the local
database, which solves #40843. (I'm not going to do that in this PR.)

Issue #40843 is a huge usability problem and time waster, and we should fix it fast instead of getting stuck in hypotheticals about controversial changes in semantics of externals.

The current state of things is that you need intricate knowledge of implementation details of Spack to understand that externals are picked up through config OR reuse concretization. The fact that externals are "installed" and can be "reused" as a consequence is an implementation detail and we shouldn't expect that's obvious to users, we should focus on their usability issues.

You can't get rid of packages.yaml externals configuration cause there will always be undetectable or incorrectly detected packages you need to manually specify as external, and config is the obvious way to do that -- being able to modify this with a text editor is far superior to special spack commands. So, config + a database with reuse semantics continues to exist also with your ideas.

Your suggestion of committing config to the database and only relying on reuse from that point on is not solving #40843 and only introduces more usability issues where users again have to be aware of this artificial concept of "uninstalling externals".

If you take packages.yaml config for externals as a filter on reusable specs things are intuitive: modified config for a previously incorrectly configured external will apply to re-concretization. There's no need to be aware of the database which makes it easy.

Yes, old "installs" of externals would continue to exist in the database even if they were misconfigured, but for users who install x ^mpich it's

1. more natural to uninstall the old x that was not working at runtime instead of "uninstalling the external" which is conceptually weird
2. in general they may not have x installed even, cause the misconfigured external likely leads to build issues -- they just reconcretize with new config and don't have to think in terms of databases at all
3. it's pretty easy to explain that external config only applies to new concretizations, cause that applies to all other config in the packages section

"but misconfigured externals continue to exist in the database" is a non-problem imo when this filter logic is extended to the local dbs.

@alalazo I think that the following:

https://github.com/spack/spack/pull/35975/files#diff-50f1229db6f3f9305546f946f508c28f829ab7e778c0a6be4e3a9f4dd6c9b1b7R3136-R3141

and specifically

and spec.external_modules == entry.get("modules")

gets close to resolving #40843: if the user modifies the modules subsection of their cray-mpich entry, then when they reconcretize, the installed external with just the two modules would not be reusable.

Are you concerned that after reconcretizing, we wouldn't actually reinstall the new spec with the new external modules (because it has the same hash)? I think that could be a problem, but I think we could infer that we need to overwrite the installed spec in that case.

This also predates #40843 and I think it's still worth merging regardless of whether it resolves that issue (although I think it might be worth discussing here in case someone thinks these changes make it harder to fix that - I don't think they do).

@scheibelp On your comment above, I am pretty sure this PR solves the issue in the description, and might solve #40843 if we use the abstract external specs as filter on the local DB. Abstract specs won't work with e.g. #30911 or with modifications that remove variants or other parts from the spec (cause the filter will be coarser).

I'm not as confident as @becker33 that we can later revert this and its natural extension to solve #40843 without affecting users, if we want to pursue a model like in #36179 + splicing. That's why I suggested #35975 (comment).

Abstract specs won't work with e.g. #30911 or with modifications that remove variants or other parts from the spec (cause the filter will be coarser).

Could we store the abstract spec as part of the db record or as an attribute of a concretized external spec? In that case, we could require equality vs. depending on .satisfies.

I don't think #36179 is strictly necessary for the issues it aims to solve. It does more than solving the referenced issues, and it's not the only solution to the them. For example for multi-valued variants the complaint is that Spack merges the list of values in config with the package defaults -- that's pretty weird, how about not doing that? Problem solved? Isn't that how package preferences for multi-valued variants work too? (I checked: that is how it works)

Regarding multi-valued variants, I do think we have an issue though that prevents this PR from being useful:

>>> spack.spec.Spec("gcc languages=c,c++").satisfies("gcc languages=c")
True

>>> spack.spec.Spec("gcc languages=c").satisfies("gcc languages=c,c++")
True

And with explicit MultiValuedVariant things you get the opposite behavior needed for this PR:

>>> MultiValuedVariant('languages', ('c')).satisfies(MultiValuedVariant('languages', ('c', 'c++')))
False

>>> MultiValuedVariant('languages', ('c', 'c++')).satisfies(MultiValuedVariant('languages', ('c')))
True

I guess you could see this as gcc +c +cxx satisfying gcc +c but not gcc +c ~cxx, there's no way to distinguish with multivalued variants you mean gcc +c ~cxx not gcc +c. Maybe what is needed is a concrete multi-valued variant where languages=c means +c ~cxx vs abstract multi-valued variant where languages=c only means +c? (we already fixed a similar issue with @ vs @=, I've also proposed languages=c vs languages:=c before iirc)

@becker33 Point 1 is already done:

Thanks, missed that when I was looking through the code earlier