On some systems, Python libraries are automatically installed to prefix.lib64. This will force them to install to prefix.lib. I'm not sure if that will break things or not, but it's something to be aware of.

@adamjstewart do you know which systems? I'll spin up a container and see if I can break things.

I've seen reports of this on opensuse, fedora, and rhel: #18520, #17126, #19546

Ok, so I think what we really need to be using is distutils.sysconfig.get_python_lib(True, prefix=prefix). That will give the platform-specific directory (which is generally used for compiled code) which is the more restrictive.

In that case, should we make the attribute in the Python package more robust?

Ok, I think this is fixed now, and handled "properly"

We currently use which in a lot of places in Spack. All of those places could benefit from that functionality. Would it be possible to instead add a install=True arg to which much like working_dir has a create=True arg? Or would that lead to unsolvable circular import issues?

I'm torn about moving this functionality into util/executable.py. It seems most useful there, but the goal with util.* is that they don't need to import from spack.*, and this definitely needs spack.spec and spack.store. I think this is something that should be discussed more before merging.

One possibility here is to use something like https://github.com/pantsbuild/pex to separate the python environment for flake8 from the rest of spack. I had started on making a PexPackage base class which would have made this relatively easy, I will evaluate whether that would be useful here.

I don't actually want the python environment separated (or at least I'm pretty sure I don't). I'm relying on the fact that we know of one working python on the system (namely, the one Spack is running in) to save huge amounts of build time. If we bootstrap flake8 without relying on the current python executable, then we have to pay the cost of building (or downloading a binary for) python and all of its dependencies. Why bother with that when we know where python is?

Sorry, when I said "python environment" I was referring to the set of installed packages. If we were to generate any PEX files e.g. for flake8, we would definitely just point pex at the resolved python executable (pex emphatically will never try to build python, it will only search for interpreters).

The reason this would come in handy is if e.g. flake8 specifies a package that conflicts with coverage or something. It would mean that we could create executables like flake8 without modifying the python environment (and therefore the PEX file would stay cached).

Does this syntax actually work? I know we've had a lot of problems with X.Y vs X.Y.Z in the past. That's why we've always had to do [email protected]:2.7.999

Good point. It works for queries, but not for building because it reads as concrete.

Wait no this should be fine -- it's used as a query, where it doesn't cause a problem, and it's used in a context manager in which the current python is an external and python isn't buildable. So there isn't the problem of concretizing to a partial version. I'll add a comment to the code to that effect though, since the reason it works is a little subtle.

Just relatedly, I had tried to introduce a new version syntax e.g. 2.6.* to give people something familiar , then immediately removed it upon user feedback about how it conflicts with shell metacharacters: #20258 (comment)

However, note that that PR also introduces 3 new version range operators !, !:, and :! to the spec syntax, which imo would provide a familiar-looking encoding of this specific common type of constraint, as 2.6:!2.7. We could even consider raising a (silencable) DeprecationError whenever we parse any version string containing e.g. .999 or .0.0.1, and optionally fix it without erroring.

I assume yaml is easier to mechanically transform than python itself, making it an easier prospect than e.g. the equivalent in pants -- see pantsbuild/pants#9434 for a PR i still haven't merged yet.

But also of note, libCST (scripts? util libraries?) would be a great way to implement both automated upgrades to package.py files (if ever needed), as well as "shading" import paths to expose multiple versions of a python package within the same running interpreter.

Python libs may be in lib or lib64, this should be made more robust by adding both.

Done

This should be the distutils of the Python dependency shouldn't it?

Oof yeah you're right. I'll run python.command.

This is already in, see #20196 . Also: local import statements should stay at the top to avoid errors where we use something that is being defined later.

Why are we adding coverage to flake8 tests? Note that on develop we won't hit the same lines of code as far as I can tell. If this is done to test the spack flake8 command, wouldn't a unit test be better?

If this is done instead to test the bootstrapping of flake8 and mypy wouldn't an explicit test be better?

The changeset in this package can be extracted and merged as a separate PR.

☝️ This would be an immediate merge if extracted.

For this TODO, I am wondering if we should perform installations of Spack dependencies in a separate store - in particular if we proceed reusing as much as possible installed packages with the ASP based concretizer. A use case I have in mind, for instance, is somebody who wants to uninstall everything:

$ spack uninstall -ay

This in my opinion shouldn't uninstall packages that are needed by Spack to run, and shouldn't make a re-build of those dependencies necessary later. In #20068 I used environments to obtain some segregation, but I understand that we may not want to use that mechanism.

Along the same lines, I am not sure we should show clingo or flake8 etc. when a:

$ spack find

command is given, or at least not the ones used for bootstrapping Spack.

I think the user should be able to uninstall things that Spack build to bootstrap itself, but we should add a dependent on those packages (from Spack) so that they cannot be uninstalled without the -f option. But (for example) Spack will bootstrap different specs depending on the python Spack runs with, and the user may want to remove some of those packages before changing pythons in their default environment (and bootstrapping new versions).

Proposal for renaming, if it encounters the taste of other maintainers:

This has weird side effects if the code is ever hit with unit tests. The tests based on builtin.mock will fail with:

    @contextlib.contextmanager
    def system_python_context():
        python_cls = type(spack.spec.Spec('python').package)
        python_prefix = os.path.dirname(os.path.dirname(sys.executable))
>       externals = python_cls.determine_spec_details(
            python_prefix, [os.path.basename(sys.executable)])
E       AttributeError: type object 'Python' has no attribute 'determine_spec_details'

since they'll pick the Python from builtin.mock. Other tests are instead installing in the user store:

$ spack find
[ ... ]
-- test-debian6-x86_64 / [email protected] ------------------------------
[email protected]

If this is done instead to test the bootstrapping of flake8 and mypy wouldn't an explicit test be better?

Is there anything specific you had in mind for this?

I don't think we should add it to the list of externals, as it would be an arbitrary modification of user config for purposes that are implementation details of Spack.

Just a minor simplification:

Maybe we can move this message to debug level?

This doesn't work for me on a stock ubuntu if I don't install setuptools first on the system:

==> Installing py-setuptools-scm-4.1.2-rd7ecfn5tjaxiwpkdnrxe2znlyleccyl
==> No binary for py-setuptools-scm-4.1.2-rd7ecfn5tjaxiwpkdnrxe2znlyleccyl found: installing from source
==> Using cached archive: /home/culpo/PycharmProjects/spack/var/spack/cache/_source-cache/archive/a8/a8994582e716ec690f33fec70cca0f85bd23ec974e3f783233e4879090a7faa8.tar.gz
==> py-setuptools-scm: Executing phase: 'build'
==> Error: ProcessError: Command exited with status 1:
    '/usr/bin/python3.6' '-s' 'setup.py' '--no-user-cfg' 'build'
See build log for details:
  /tmp/culpo/spack-stage/spack-stage-py-setuptools-scm-4.1.2-rd7ecfn5tjaxiwpkdnrxe2znlyleccyl/spack-build-out.txt

This means installing python3 and python3-setuptools with apt. If I install python3-setuptools I get a different error:

Traceback (most recent call last):
  File "/home/culpo/PycharmProjects/spack/opt/spack/linux-ubuntu18.04-broadwell/gcc-10.1.0/py-flake8-3.8.2-sprazagnhxqodvnn6mzfwtkheligd24v/bin/flake8", line 6, in <module>
    from pkg_resources import load_entry_point
  File "/usr/lib/python3/dist-packages/pkg_resources/__init__.py", line 3088, in <module>
    @_call_aside
  File "/usr/lib/python3/dist-packages/pkg_resources/__init__.py", line 3072, in _call_aside
    f(*args, **kwargs)
  File "/usr/lib/python3/dist-packages/pkg_resources/__init__.py", line 3101, in _initialize_master_working_set
    working_set = WorkingSet._build_master()
  File "/usr/lib/python3/dist-packages/pkg_resources/__init__.py", line 574, in _build_master
    ws.require(__requires__)
  File "/usr/lib/python3/dist-packages/pkg_resources/__init__.py", line 892, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/usr/lib/python3/dist-packages/pkg_resources/__init__.py", line 778, in resolve
    raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'flake8==3.8.2' distribution was not found and is required by the application
==> Error: Flake8 style checks found errors
==> Error: style: mypy is not available in path, skipping
spack style found errors.

Once #20652 is in we can modify this to:

Docstring should be updated too.

Same question as above on the opportunity to run coverage on spack style.

☝️ This would be an immediate merge if extracted.

We may add something like the following, to try first installing from binary cache:

diff --git a/lib/spack/spack/bootstrap.py b/lib/spack/spack/bootstrap.py
index 9eea069..59b4597 100644
--- a/lib/spack/spack/bootstrap.py
+++ b/lib/spack/spack/bootstrap.py
@@ -78,7 +78,11 @@ def make_module_available(module, spec=None, install=False):
         # We will install for ourselves, using this python if needed
         # Concretize the spec
         spec.concretize()
-    spec.package.do_install()
+    try:
+        spec.package.do_install(cache_only=True)
+    except Exception as e:
+        tty.debug(str(e))
+        spec.package.do_install()
 
     module_path = os.path.join(spec.prefix,
                                spec['python'].package.site_packages_dir)

and if that fails revert to install from sources. Trying the cache only installation first allows us to not install the build dependencies if they are not needed, thus speeding up the bootstrapping process.

It's a bit worse than the diff above, since at the moment calling do_install(cache_only=True) raises a SystemExit on failure that requires a BaseException to be caught. We should probably refactor the call such that the exception raised is something else.