1. Okay, I am a bit confused.
   Is it really good to keep such a big list in two places? It's even arch dependant.
   Even for simple things, I miss this duplication? (say config:db_lock_timeout).
   Why do you have etc/spack/defaults/ anyway? For documentation?
2. The other place using the prefix inspections is lib/spack/spack/modules/common.py. And it has a {} as well.
   So if a duplication is actually wanted, we should change that place as well.

About moving this into some other place: Let's discuss that in a different issue/PR?

In short: I agree with Greg

As for why:

/etc/spack/defaults/ sets values that are required for Spack to run properly (i.e. they require some setting) but that the user may want to adjust themselves; the file is there for the user to scan if they want to know what they can configure about Spack. Generally speaking you are right that the file isn't strictly necessary because most (all?) places in the code that pull from it also choose their own default value, but removing it would make it harder to see what can be configured.

On the other hand, ideally the accidental deletion of the file wouldn't lead to abrupt terminations of Spack; therefore the code also sets defaults.

So in this case I think the duplication is "for a good cause".

Finally, regarding the module configuration default choice of {}: the user can entirely disable modules by deleting the module configuration, and in this case I think {} is a sensible default (Spack still runs in this case, it just doesn't produce any modules). I think the handling of module configuration is a sensible exception but perhaps @becker33 would disagree.

It's even arch dependant.

Could you add an example of this?

Other questions/notes:

• Are you doing this because the lack of consistency was leading to an error (i.e. something was working in a module-managed environment that wasn't in a Spack environment)?
• Additionally (not specifically related to the concerns brought up by you or Greg): code that modifies variables like PATH/LD_LIBRARY_PATH etc. based on rigid prefix patterns can miss some of the nonstandard layouts that Spack packages install into (e.g. why some packages have custom a custom libs property). We want module prefix inspections to be customizable by the user so it's hard to think of a syntax that allows for that in combination with allowing packages to customize inspections themselves. I'm not sure if the code which modifies Spack environments should be bound by the same constraints.

[… etc/spack/defaults/ and code duplication …]

Let me see, if I understand that correctly:

• etc/spack/defaults/ has all the settings needed to make spack work properly.
• If the user deletes that, the code should have fallback defaults.
• Those fallback defaults don't need to one-by-one duplicate etc/spack/defaults/. BUT they should be sane enough to make spack work somewhat. Not necessarily properly in all cases.
• My conclusion: Fallback defaults in code should be the minimal set to make spack work at all.

Sounds fine to me!

Finally, regarding the module configuration default choice of {}: the user can entirely disable modules by deleting the module configuration, and in this case I think {} is a sensible default (Spack still runs in this case, it just doesn't produce any modules). I think the handling of module configuration is a sensible exception but perhaps @becker33 would disagree.

Yes: {} seems to be the super-minimal fallback in code. And that's what my changes also has.

If we can settle on {} as the fallback in code: I am already done.

It's even arch dependant.

Could you add an example of this?

Linux: lib -> LD_LIBRARY_PATH
macOS: lib -> DYLD_FALLBACK_LIBRARY_PATH (or whatever the correct name was)
AIX: lib -> LIBPATH (I think, too long ago!)

For Linux/macOS this is in etc/spack/defaults/.

Other questions/notes:

• Are you doing this because the lack of consistency was leading to an error (i.e. something was working in a module-managed environment that wasn't in a Spack environment)?

In some sense: Yes.

In our setups, we want to use spack load. But we don't want it to set LD_LIBRARY_PATH (see #3955 for some reasons). With modules, we could achieve that using config overriding, which makes it easy. But for spack load, we'd need to maintain local patches to spack. And we try to reduce those as much as possible.

But anyway, having nice DRY code is a goal in itself. I think, the affected parts of spack were even desynced at some point. So this should improve maintainability.

• Additionally (not specifically related to the concerns brought up by you or Greg): code that modifies variables like PATH/LD_LIBRARY_PATH etc. based on rigid prefix patterns can miss some of the nonstandard layouts that Spack packages install into (e.g. why some packages have custom a custom libs property). We want module prefix inspections to be customizable by the user so it's hard to think of a syntax that allows for that in combination with allowing packages to customize inspections themselves. I'm not sure if the code which modifies Spack environments should be bound by the same constraints.

The goal of this PR is really to only improve DRY/maintainability and keeping functionality nearly the same (well, improve spack load).
I would prefer, if we could move this into a new issue (and possibly then a PR). Just open one and ping me! I think, there are various points to discuss there. Even some of the stuff of #3955 might play into this.

Are you doing this because the lack of consistency was leading to an error (i.e. something was working in a module-managed environment that wasn't in a Spack environment)?

In some sense: Yes.
In our setups, we want to use spack load. But we don't want it to set LD_LIBRARY_PATH (see #3955 for some reasons). With modules, we could achieve that using config overriding, which makes it easy. But for spack load, we'd need to maintain local patches to spack. And we try to reduce those as much as possible.

Based on this I don't think this is about deduplication but rather control of the prefix inspections done by the spack load command (and in that sense I wouldn't consider this purely a refactor, and in that sense definitely seems worth supporting).

Those fallback defaults don't need to one-by-one duplicate etc/spack/defaults/. BUT they should be sane enough to make spack work somewhat. Not necessarily properly in all cases.

I don't think my examples contained a case of "improper" behavior: e.g. for modules, if you specify nothing then it makes sense to generate no modules.

However, if you specify nothing for prefix_inspections, does it make sense for spack load to do nothing? Presumably if someone is invoking spack load they assume it will have some effect. Minimally I think a warning message would be sensible if we end up calling prefix_inspections and set nothing.

It's even arch dependant.

Could you add an example of this?

Linux: lib -> LD_LIBRARY_PATH
macOS: lib -> DYLD_FALLBACK_LIBRARY_PATH (or whatever the correct name was)
AIX: lib -> LIBPATH (I think, too long ago!)

Good point! That being said though I think if we are going to use the config to handle prefix inspections, then that means that we need to make sure that prefix_inspections from the modules configuration reflects these OS differences. Right now I see that etc/spack/defaults/darwin/modules.yaml does not appear to add DYLD_FALLBACK_LIBRARY_PATH for lib so I think that should change. In other words, this PR should update the /etc/spack/defaults/.../modules.yaml files so the behavior stays the same as before (unless you modify those files or override the config).

In short:

• update default config files (particularly OS-custom ones)
• add warning message to unconditional_environment_modifications if prefix_inspections is empty

In short:

• update default config files (particularly OS-custom ones)

[…]
Good point! That being said though I think if we are going to use the config to handle prefix inspections, then that means that we need to make sure that prefix_inspections from the modules configuration reflects these OS differences. Right now I see that etc/spack/defaults/darwin/modules.yaml does not appear to add DYLD_FALLBACK_LIBRARY_PATH for lib so I think that should change. In other words, this PR should update the /etc/spack/defaults/.../modules.yaml files so the behavior stays the same as before (unless you modify those files or override the config).

Okay, what am I missing?

https://github.com/spack/spack/blob/develop/etc/spack/defaults/darwin/modules.yaml#L18-L21

That looks quite correct to me? It has the FALLBACK one, which AFAIK is the one good for macos?

We don't yet support aix, right?

For Linux it's also fine.

I think, that points is done?

In short:

• add warning message to unconditional_environment_modifications if prefix_inspections is empty

done.

It is a long-standing practice in Spack that the code defaults are expected to be exactly the defaults provided in /etc/spack/defaults. Among other reasons, it makes writing unit tests for Spack substantially easier. I do not think there is anything about this PR that warrants changing that behavior.

That is not a criticism of the rest of this PR, I think it's a good change that we should be making. We just need to keep the code defaults updated in keeping with the changes.

For example, if someone deleted /etc/spack/defaults/modules.yaml prior to this PR, merging this PR should not change their existing behavior of spack load.

I think then that the way you would address

Are you doing this because the lack of consistency was leading to an error (i.e. something was working in a module-managed environment that wasn't in a Spack environment)?

In some sense: Yes.
In our setups, we want to use spack load. But we don't want it to set LD_LIBRARY_PATH (see #3955 for some reasons). With modules, we could achieve that using config overriding, which makes it easy. But for spack load, we'd need to maintain local patches to spack. And we try to reduce those as much as possible.

while following #18260 (comment)

is to explicitly add configuration that overrides lib/lib64 settings to exclude LD_LIBRARY_PATH (in that case the default will not be used).

is to explicitly add configuration that overrides lib/lib64 settings to exclude LD_LIBRARY_PATH (in that case the default will not be used).

modules:
  prefix_inspections::  # double colon to override defaults
    'bin': ['PATH']
    ... # add the rest of the ones you do want here

That will satisfy the use case if I'm understanding correctly

Well, I am again confused.

For normal modules the in-code-default is {}. So this "long standing practice" wasn't adhered to very closely, it seems to me?

I think, we should settle on two questions:

1. Should the spack load module-like system, and the other module systems behave the same, or at least as closely as possible? If it should behave differently, there should IMHO be a reason given.
2. Is deleting etc/spack/defaults/ a supported use case?
   • I personally think, there's no need to support that use case at all, because we have the double colon overriding :: (see previous posting).
   • If it is a supported use case, what is the expected behaviour?

For normal modules the in-code-default is {}. So this "long standing practice" wasn't adhered to very closely, it seems to me?

If you are referring to

def configuration():
    return spack.config.get('modules', {})

in modules/common.py, then I think my explanation in #18260 (comment) addresses this (and more on that below here).

Should the spack load module-like system, and the other module systems behave the same, or at least as closely as possible? If it should behave differently, there should IMHO be a reason given.

Generally they should behave the same although again #18260 (comment) explains at least one context where they should differ: the spack load module system is executed in the context of a user invoking spack load (and presumably they only run this command if they want to change their shell variables); the "other module systems" AFAIK refers to Spack's automatic module generation logic - if the user overrides that to be empty then presumably they don't want to generate any modules (so I think the default of {} makes sense in that case).

Is deleting etc/spack/defaults/ a supported use case?

We want Spack to do something sensible if this gets accidentally deleted somehow. We would not suggest that the user delete the file to change the merged configuration.

If it is a supported use case, what is the expected behaviour?

I think Greg's assertion is that if a user deletes, etc/spack/defaults/, then Spack should behave the same as if it was there; this is only in the context of spack load (as discussed above, behaving the same is arguably not the right thing to do in all cases but seems sensible here).

Thanks for the explanation!

So if I understand that correctly, for spack load we need a very good fallback. So let's just fallback to the full list.

@ChristianTackeGSI Thanks for all the work on this PR!