Would it be possible to move pure style changes like this into a separate commit?

These style changes (replacing join_path with os.path.join) are currently mixed into 2557ea6 - could they go in their own commit?

I don't think this particular case is important (2557ea6 is simple even with that mixed in) but generally prefer that pure-style changes go in their own commits (and replacing join_path is close enough to a pure-style change to qualify IMO).

Is this TODO stale? If not then that sounds like a fair amount of additional work.

would look like:: -> would look like:

This syntax (e.g. config:dirty) appears to be new. I had to scan through the commit messages to find 2b0c8f6 which explains it's purpose. Presumably the line

Simplify the spack.config interface to allow getting/setting either entire YAML files or specific parts (single variables).

is relevant? IMO it's worth mentioning the syntax there.

Did you mean to call this the config scope here?

EDIT: I was assuming this would be the config section

configuration (typo)

Do you have to test spack.config vs. a Configuration object?

What happens if this depends on spack.config?

Still curious about this. I assume it has more to do with cyclic dependencies than initialization of state, at least for spack.spec. I'm generally wary of function-local imports - I suspect they are more difficult to maintain.

Minor nitpick: os.path is documented as a module, and usually I import it directly if I need to use any of its functions. Looking for best practices took me down into another python rabbit-hole.

Not sure we should do it in this PR, but I think it's worth swapping user scope with site scope, and make site the default for configuration. Based on my experience using multiple Spack versions at the same time, writing by default to user config is very likely to create conflicts if the configuration file format changed in the meanwhile.

I think this is good but for another PR.

👍 to this

Isn't __repr__ usually supposed to return a string such that:

b = eval(repr(a))
assert b == a

?

typo: confiugration

Do you mean lists of objects? Something like:

spack.config.get('config:tests:0:url')

to get the url key from the item at index 0 of a list of things that looks like:

[{ url: 'foo' },
 { url: 'bar'}]

? Or are you referring to another behavior that is not handled?

I mean something like that, but I think we'd need to differentiate the syntax, e.g.:

spack.config.get('config:tests[0]:url')

The keys following : are assumed to be strings, and result in index[key] in the Python code. The indexes for lists are integers, and you'd need to convert them before indexing. Yes, we could do that by looking at the type of structure we're indexing, but I think the above is clearer anyway because you cana clearly see what's expected to be a list and what's expected to be a dict in the path specifier.

I'm basically just saying we're not doing anything to address the list case currently, so you can't ask for, e.g.:

spack.config.get('repos[2]')

This function is used frequently so IMO the TODO should be a # comment vs. living in the docstring.

ok moved

Don't know how much this would be a problem in reality, but I think the semantic of this class is kind of confusing. If I read the code correctly, by default when we get a value we retrieve a merged view of all the scopes. Instead, when we set a value, we set it in a particular scope.

Am I wrong saying that this means that:

# We set the value in the highest precedence scope by default
spack.config.set(somepath, a) 

# We merge all the scopes by default
b = spack.config.get(somepath)

# This might be False, e.g. if 'somepath' is a dict spread over scopes
assert a == b

might happen? In case is this the behavior we want?

This would never be false, unless you specified a scope in the first spack.config.set. FWIW, this new syntax doesn't actually change the semantics of working with the config system. It just makes it easier to partially update something without getting the entire dictionary for a level.

Say we have two scopes for foo.yaml:

# first scope
foo:
   bar: 5
# second scope
foo:
    baz: 6

Am I wrong on the following snippet:

spack.config.set('foo', {'foobar': 6}) 
b = spack.config.get('foo')
assert b == {'foobar': 6, 'baz': 6}

?

I see what you're saying -- I think I was wrong. I actually think b == {'bar': 5, 'foobar': 6, 'baz': 6} should be true, but we should support something like spack.config.set('foo:') for consistency (where the : means overwrite).

I guess the question is whether I should address that now or whether that can be another PR. I do think that the path syntax would be useful, e.g., for allowing these expressions to be used with spack config get.

Nice addition!

Is this import needed for something?

nope. fixed.

I see you are using this pattern in several places. As a minor improvement, did you consider using something like:

class LazyObject(object):
    def __init__(self, factory):
        self.factory = factory
        self._instance = None
    
    @property
    def instance(self):
        if self._instance is None:
            self._instance = self.factory()
        return self._instance
    
    def __getattr__(self, name):
        return getattr(self.instance, name)
  
    def __str__(self):
        return str(self.instance)

    def __repr__(self):
        return repr(self.instance)

? I think this could simplify a the code. For instance here we could write:

def _misc_cache_factory():
    path = spack.config.get('config:misc_cache')
    if not path:
         path = os.path.join(spack.paths.user_config_path, 'cache')
    path = canonicalize_path(path)
    return FileCache(path)

misc_cache = LazyObject(_misc_cache_factory)

and in client code refer to spack.caches.misc_cache instead of spack.caches.misc_cache()

I like it!

To be consistent with other changes we should also change join_path to os.path.join here, and in general in other tests that have been touched by this PR. In the end we want to encourage the use of join_path only within package recipes, right?

Minor, but why swapping fnmatch with os? Can't we keep in general alphabetical ordering within the three groups of imports that appear in PEP8?

I think it's a good idea but if we want that we should enforce it. I think my natural inclination is to put things like sys and os first. Can we do another PR where we alphabetize imports and put in a test to keep them that way? Seems reasonable to me to prevent import churn.

I like this change in the handling of tests! Maybe later we could extend it to accept a csv list, or someting like that, if we need the extra flexibility.

Say we have two scopes for foo.yaml:

# first scope
foo:
   bar: 5
# second scope
foo:
    baz: 6

Am I wrong on the following snippet:

spack.config.set('foo', {'foobar': 6}) 
b = spack.config.get('foo')
assert b == {'foobar': 6, 'baz': 6}

?

Why do we need wrappers to mask the singleton? Can't we avoid this?

@scheibelp thinks that this is more readable. I personally want to keep the wrappers to a minimum as I think it's better to have one way to access the function, but I also see the readability point. I specifically tried not to add too many wrappers, but added a few for common cases. For config in particular I think get/set read a lot better as wrappers, but I don't have too much of an opinion on this.

My preference is consistency. Earlier we had

config.get('packages:...')

and

config.get_configuration().scopes

(get_configuration() has since been renamed to config()). Having two different ways of interacting with the singleton was confusing IMO. I think masking the singleton is OK as long as it's clear what is going on when someone looks at config.py specifically.

Typo: file

I am not sure I get the logic of this part. Is this used only within unit tests to disable building?

@alalazo yes - perhaps the docstring for the function could be clearer?

Unit tests can override this function to perform checks after Package.install (and all post-install hooks) has run but before the database has been updated. The overridden function may indicate that the install procedure should terminate early (before updating the database) by returning False or any value such that bool(result) is False

I think I addressed this below

If we refactor this as we've discussed in the past, so that repo gets the class and not an instance,

What do you mean by this?