@spackbot fix style

Let me see if I can fix that for you!

I was able to run spack style --fix for you!

==> Running style checks on spack
  selected: isort, black, flake8, mypy
==> Modified files
  lib/spack/spack/environment/environment.py
  lib/spack/spack/test/cmd/dev_build.py
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/test/cmd/dev_build.py
All done! ✨ 🍰 ✨
1 file reformatted, 1 file left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 576 source files
  mypy checks were clean
==> spack style checks were clean

I've updated the branch with style fixes.

Can you implement it along the lines of

def _get_overwrite_specs(
    concrete_specs: List[spack.spec.Spec], database: Optional[spack.database.Database] = None
) -> List[str]:
    """If a develop spec has been modified, we need to overwrite it and its parents.
    Also ensure that the parents of installed develop specs are overwritten if they
    are older than the installation time of the develop spec.

    Args:
        database: The database to use. If None, the default database is used.

    Returns:
        List of dag hashes of specs that need to be overwritten."""
    db = database or spack.store.STORE.db

    # Get all specs in topological order.
    specs: Iterable[spack.spec.Spec] = reversed(
        list(traverse.traverse_nodes(concrete_specs, order="topo", key=traverse.by_dag_hash))
    )

    # max_time[spec.dag_hash()] is the maximum installation time dev deps; or inf
    # if any of its deps needs to be overwritten.
    max_time: Dict[str, float] = {}
    hashes_to_overwrite = []

    for spec in specs:
        key = spec.dag_hash()
        upstream, record = db.query_by_spec_hash(key)
        dep_time = max_time.pop(key, 0.0)

        if not upstream and record:
            # Case 1: spec is installed locally, and a newer dev dep is in the subdag.
            if record.installation_time < dep_time:
                hashes_to_overwrite.append(key)
                dep_time = math.inf
            else:
                path = spec.variants.get("dev_path", None)
                if path:
                    mtime = fs.last_modification_time_recursive(path.value)
                    # Case 2: this spec is a develop spec, and it has been modified.
                    if mtime > record.installation_time:
                        # Have to reinstall this develop spec.
                        hashes_to_overwrite.append(key)
                        dep_time = math.inf
                    else:
                        # Case 3: the develop spec is already installed, dependents
                        # need to be overwritten if they are older than the installation
                        dep_time = max(dep_time, record.installation_time)

        # Propagate max dep install time to dependents
        for dependent in spec.dependents():
            dep_key = dependent.dag_hash()
            if max_time.get(dep_key, 0.0) < dep_time:
                max_time[dep_key] = dep_time

    return hashes_to_overwrite

I think it's more clear if you topo sort and then propagate (a) install times of up-to-date develop dependencies or (b) infinity for out of date develop deps. Maybe split it into two functions to avoid the deeply nested control flow.

That way we also don't check O(develop spec) modification times if say one common leaf node has changed, and checking one would be enough.

Also, with the above structure ^ could you refactor the test so that it takes a few us/ms instead of seconds? Construct a dev spec, mark it concrete, create a db, add it with particular install time. That way you don't need to concretize, hit the filesystem as much, or use sleep 😆

Btw, I'm also thinking that we should maybe reverse the default order of topo sort, cause if you look at the implementation it already does a reverse, and the call site commonly re-reverses it, like ^

I still would say it's better to combine the two passes.

I have now done so: the method will now avoid checking timestamps on dependents if a dependency has already met the criteria for overwriting.

The test is updated now as well (and moved, since it isn't exercising the dev-build command).

@haampie can you review again?

Some small requests, and then let's get this pr in.

FWIW, I'm generally not a fan of using Spec objects as keys in dicts and sets. I guess it's OK here since all specs are concrete, but still better to reduce the number of times we call Spec.__hash__ and Spec.__eq__.

I have fixed the bug mentioned in #35442 (comment)

Why don't you assign transitive_dev_install_time_child = transitive_dev_install_times[spec] first and then conditionally update it, instead of calling transitive_dev_install_times[spec] 3 times?

Instead of looking up this value yet again, why not return it from update_transitive_dev_install_times.

In fact you can also delay looping over dependents() until after this branch, since it's redundant if this branch is followed.

I have tried to refactor the code with these requests in mind as well, but as for why I did it the way I did it:

• I consider it "free" to do O(1) operations that are O(1) any time I write a function; if intermediate values become temporarily irrelevant, I prefer to discard and recompute them, since carrying them around complicates functions.
• I also dislike inferring state from underlying variables. For example when we used InstallRecord.installed vs. Database.installed we are subverting the API of Database: we know asking if record and record.installed is the same as Database.installed, but not every person who comes along and reads this code will immediately understand that.

FWIW, I'm generally not a fan of using Spec objects as keys in dicts and sets. I guess it's OK here since all specs are concrete, but still better to reduce the number of times we call Spec.hash and Spec.eq.

• Spec.__hash__ is cached for concrete specs, if you call it once, all future calls are nearly instantaneous.
• I can see a case made for __eq__, although I am personally unconvinced without profiling. If it is truly a major time sink in Spack, could we make it fast by maintaining a global cache like:

class SpecEqCache:
  def __init__:
    self.cache = defaultdict(list)

  # every concrete spec that calls _cmp_iter can call this and set a property
  def eq_id(self, spec)
    hash_list = self.cache(spec.dag_hash)
    x = hash_list.index_of(spec)
    if not x:
      hash_list.append(spec)
      x = len(hash_list) - 1
    return x

?