This feels like a matter of interpretation: I don't normally have an issue with considering external packages as installed. Does something go wrong when external packages are considered installed?

Would this also imply there needs to be consideration about when a user adds an external package but doesn't explicitly install it? In this case has_prefix would be True and db_says_installed would be False. As of now the tutorial on external packages doesn't explicitly instruct the user to invoke spack install for external packages (see: http://spack.readthedocs.io/en/latest/tutorial_configuration.html#external-packages). If it's going to report installed = False then there should be a warning when the values don't match: I think users may have logic which checks for Package.installed; from skimming through the codebase I don't think that exists now, but I think it's reasonable for users to check for that.

Also the unit test looks good although IMO instead of directly invoking database.install it would be best to invoke Package.install to confirm that the external ends up installed in the database (if you agree then it also might make sense to move this test to the test/install.py module).

This feels like a matter of interpretation: I don't normally have an issue with considering external packages as installed. Does something go wrong when external packages are considered installed?

Nothing super-severe, but if you consider external packages installed without being registered in the DB:

1. spack find won't show them
2. Module files are not generated for them
3. spack install will take action for something that is considered already installed

Would this also imply there needs to be consideration about when a user adds an external package but doesn't explicitly install it?

Probably a couple of lines in the doc that inform users of the discrepancies above? If you want I can try to add a sentence somewhere to show how installing an external package differs from installing a regular one.

I think users may have logic which checks for Package.installed; from skimming through the codebase I don't think that exists now, but I think it's reasonable for users to check for that.

That's how I hit this issue. I wrote a command to help scripting a deployment procedure (#7899) and noticed that external packages were always considered installed, even if they were not explicitly spack installed. That prevented logic like "if X is not installed, then install it (which also means generate module files, etc.)" from working.

Also the unit test looks good although IMO instead of directly invoking database.install it would be best to invoke Package.install to confirm that the external ends up installed in the database (if you agree then it also might make sense to move this test to the test/install.py module).

👍

@scheibelp I just double checked this:

Also the unit test looks good although IMO instead of directly invoking database.install it would be best to invoke Package.install to confirm that the external ends up installed in the database

and the database fixture is done in such a way to call:

def _install(spec):
    s = spack.spec.Spec(spec)
    s.concretize()
    pkg = spack.repo.get(s)
    pkg.do_install(fake=True)

on its install method (thus I think we are done here, right?). Let me know if you still want to move the test. I have no objection to it in case.

That's how I hit this issue. I wrote a command to help scripting a deployment procedure (#7899) and noticed that external packages were always considered installed, even if they were not explicitly spack installed. That prevented logic like "if X is not installed, then install it (which also means generate module files, etc.)" from working.

It should be ok to invoke Package.do_install on an already-installed package, but I agree that if not X.installed: X.do_install is sensible.

IMO there are two competing desires:

• Users that add externals should expect that Spack considers them installed without invoking Spack install on them (admittedly nothing that I know of now depends on this, I just think it's reasonable)
• Assuming that one-time actions are tied to installation state, the user should be able to trigger those actions (which is your issue in Added a new command to filter input specs based on their properties #7899)

Would there be an issue with having Package.installed automatically update the DB record etc. if it is missing?

Probably a couple of lines in the doc that inform users of the discrepancies above?

I'm worried this would be fairly common and it would be nice to handle things automatically if possible.

and the database fixture is done in such a way to call... on its install method (thus I think we are done here, right?)

I don't think there's a test which confirms right now that invoking Package.do_install will appropriately add an external to the database. Even if we know that Package.do_install is called indirectly by the mock db I think that (a) it adds work for people to verify this and (b) the behavior of the mock db may change in the future.

Would there be an issue with having Package.installed automatically update the DB record etc. if it is missing?

I see two valid reasons why not doing that:

1. Entries in packages.yaml act more like constraints than full specs (there's no 1:1 mapping from an entry to a single spec that needs to be installed)
2. It would be surprising to have a querying property with side-effects like writing module files on disk or modify the DB

By 1. I mean that, for instance, the configuration file in #8036:

packages:
  openssl:
    buildable: False
    paths:
      openssl@system: /usr/lib

says that whenever openssl is needed it must be at version @system, but leaves open the compiler and target part of the spec. In this sense it's just prescribing a constraint to the concretizer, rather than identifying a single spec that was installed manually outside of Spack. If we need openssl %gcc and openssl %intel we'll end-up using the same external installation for both (and this is completely fine).

Regarding 2. I think I would really be surprised (not positively) by a behavior like:

# Evaluating the condition might write module files to disk for external packages
# or modify spack DB
if openssl.installed:
   # This will never be hit by external packages, even if they don't appear in `spack find`
   pass

TLDR I would stay explicit, and let people install via spack install <specs> whatever they need without having to differentiate between external packages and packages managed by Spack.

I don't think there's a test which confirms right now that invoking Package.do_install will appropriately add an external to the database. Even if we know that Package.do_install is called indirectly by the mock db I think that (a) it adds work for people to verify this and (b) the behavior of the mock db may change in the future.

There is, but is done indirectly via the database fixture:

This was added as part of #1167

There is, but is done indirectly via the database fixture:

I think there should be a test to ensure that Package.do_install does this directly: if someone had forgotten to update the database in Package.do_install, the behavior would be incorrect but test_external_entries_in_db would still pass.

[the packages.yaml entry] says that whenever openssl is needed it must be at version @system, but leaves open the compiler and target part of the spec. In this sense it's just prescribing a constraint to the concretizer, rather than identifying a single spec that was installed manually outside of Spack.

I think we support that mainly because we don't want to necessarily force users to be specific about the details. Even though we weren't specific, the external openssl does actually have a specific compiler that it was built with. Not specifying the compiler could lead to ABI incompatibilities. You mention

this is completely fine

so I'm assuming perhaps this works out for openssl specifically. I don't think it's guaranteed to work in general.

It would be surprising to have a querying property with side-effects like writing module files on disk or modify the DB

I agree that would be strange.

I want to make sure that if we require users to explicitly install externals, that it's clear to users what is going wrong if an external is considered not-installed. Perhaps something close to that would be to print a warning message when a not-installed external spec is encountered.

Actually wouldn't this be rare? If the external appears as a dependency in a spec DAG and that DAG is installed, the database should be updated with the external (or I think that's how it works from looking at Package.do_install).

In other words I'm asking: when do you encounter an external that hasn't been registered in the DB?

It may also be strange to users to have to explicitly install the same external multiple times with different specs if they want Spack to treat it as installed in different contexts (DAGs).

I think there should be a test to ensure that Package.do_install does this directly: if someone had forgotten to update the database in Package.do_install, the behavior would be incorrect but test_external_entries_in_db would still pass

This, I think, as been taken care of in the latest rebase - at least for the modifications that are part of the PR.

Even though we weren't specific, the external openssl does actually have a specific compiler that it was built with. Not specifying the compiler could lead to ABI incompatibilities.

Agreed, but with the caveat that I don't see it as a problem. To explain better my point of view: I interpret the semantic of entries in packages.yaml somewhat like how the keyword unsafe is interpreted in certain languages: it lets you take control over some dependencies, but then you are on your own all the way down from there (i.e. without the checks that Spack does for you to ensure you have a consistent DAG). I think this is sensible as we can't expect a user to fill in all the details for a spec that Spack would have filled for him.

In other words I'm asking: when do you encounter an external that hasn't been registered in the DB?

It may also be strange to users to have to explicitly install the same external multiple times with different specs if they want Spack to treat it as installed in different contexts (DAGs).

For openssl you're completely correct: it will be installed as a dependency when needed. The issue solved here arises, in my experience, when you are deploying a complete environment on clusters and is specific to third party applications that are distributed as binaries.

At least in our case, these applications (like gaussian, adf, etc.) are installed manually in a folder separate from the installations managed by Spack. Yet we want our users not to be aware of the difference and provide them a module file as we do for all the other libraries and applications. This module file, for consistency and convenience, should be generated by Spack. To generate it we need to install the external application explicitly and for as many different targets as we have on our clusters.

@scheibelp Pinging you as you told me to do 😄

ping

@scheibelp Should be done. Let me know if there's anything else you want changed.

Thanks!