Automatically find externals by scheibelp · Pull Request #15158 · spack/spack

scheibelp · 2020-02-21T22:55:09Z

Add a spack external find command which searches through $PATH and tries to create a packages.yaml of external packages. This focuses on finding build dependencies (currently only cmake).

To make a package discoverable with spack external find the package must define an .executables class attribute: a collection of executable names. If Spack finds these executables, it will create a packages config entry for them. If the package also defines .determine_spec_details, the executables will be passed to that function, which can generate a more-specific packages.yaml entry (including version, variants or whatever else the user decides to determine).

This currently outputs the relevant yaml configuration to the terminal and doesn't have an option to generate a packages.yaml file.

Closes #2507
Fixes #13198
Fixes #2116

… static strings

alalazo · 2020-03-11T13:35:56Z

I think the core proposal in #1795 might be of interest for a broader discussion. The basic idea is that a package that is detected as an external can be symlinked into a Spack owned directory. This would ensure that:

We don't introduce spurious dependencies by adding system folders as prefixes
We have a correct modelling of what needs to be detected for any external package that supports the detection API (if a critical component is missing, recipes are more likely to fail if we are out of system paths)

scheibelp · 2020-03-21T00:22:16Z

[This would ensure that] We have a correct modelling of what needs to be detected for any external package that supports the detection API (if a critical component is missing, recipes are more likely to fail if we are out of system paths)

I would like to give users the option to specify a subset of files (e.g. if a cmake exe is available on the system, I assume it is likely to work). Allowing users to specify more required files could be useful.

[This would ensure that] We don't introduce spurious dependencies by adding system folders as prefixes

I think this can be treated as a separate issue: we already allow users to add external packages which are located in system folders (and in that case they enter the system folder as the prefix). Spack's logic already handles this case so it should be the same for automatically adding externals with system prefixes.

The basic idea is that a package that is detected as an external can be symlinked into a Spack owned directory.

Overall I'm not sure of the benefit of this (vs. detecting and using system prefixes). I admit it sounds good to me, but I think we already need to handle the existence of system prefixes, so I'm not sure if separating the files into a different prefix is beneficial.

alalazo

I left a few comments, let me know what you think.

lib/spack/spack/cmd/external.py

…o test the function

…package (e.g. with different versions)

…ce of multiple packages

…ances of a package (e.g. two instances of CMake stored in different paths)

…y (the logic was trying to os.listdir on it)

… as external packages

…g to create external package configs

… relevant exes found in that prefix; it can return multiple specs from a single call (although generally the package config can't distinguish the specs). determine_spec_details can return a single spec if the user doesn't want to implement the logic to detect multiple versions in a single prefix

scheibelp · 2020-04-29T23:41:59Z

@tgamblin @alalazo this is ready for another look

scheibelp · 2020-04-29T23:47:44Z

I should add a section on things that are not yet addressed but I presume they could be handled with a later PR:

Parallelize the execution of determine_spec_details across different packages: Automatically find externals #15158 (comment)
Ensure repeatable results in the case of conflicts: Automatically find externals #15158 (comment)
Handle external packages with atypical layouts (i.e. not <prefix>/, <prefix>/bin, <prefix/lib>, etc.): Automatically find externals #15158 (comment)
(EDIT: moved from Automatically find externals #15158 (comment)) Add a new scope to register automatically found externals at (at the moment, this may not be critical as I have added logic/testing to ensure that existing spec entries are not overwritten)
(done now) ~~Use re.compile on all regexes (actually that is simple enough to do here but isn't done yet): Automatically find externals #15158 (comment)~~

All other concerns are addressed at this point.

…ndidate exe was not an instance of the package was triggering incorrectly. remove confusing and incorrect use of for/else python syntax

alalazo

LGTM. I think all the other points mentioned in the discussion can be implemented in subsequent PRs. I left a couple of minor comments.

alalazo · 2020-05-01T18:12:39Z

lib/spack/spack/cmd/external.py

+    groups = defaultdict(set)
+    for p in paths:
+        groups[os.path.dirname(p)].add(p)
+    return groups.items()


I think we can return:

result = sorted([(prefix, sorted(s)) for prefix, s in groups.items()])

to solve the TODO below

That would do it, although the TODO I have in the code mentions that it might be better to do it based on order of prefixes in PATH (so the results are repeatable and follow an intuitive rule).

alalazo · 2020-05-01T18:16:54Z

var/spack/repos/builtin/packages/cmake/package.py

+            match = re.search(r'cmake.*version\s+(\S+)', output)
+            if match:
+                version_str = match.group(1)
+                return Spec('cmake@{0}'.format(version_str))


Maybe we can just:

Suggested change

return Spec('cmake@{0}'.format(version_str))

return 'cmake@{0}'.format(version_str)

and move the logic that parses the string into Specs in the command, as you suggested in a comment above.

tgamblin

@scheibelp: this LGTM. Can you please add documentation before we merge it?

scheibelp · 2020-05-05T20:45:24Z

Can you please add documentation before we merge it?

@tgamblin I have added documentation in 8af0872

tgamblin · 2020-05-05T23:53:35Z

lib/spack/docs/build_settings.rst

+Specific limitations include:
+
+* A package must define ``executables`` and ``determine_spec_details``
+  for Spack to locate instances of that package.


This needs further explanation in the packaging guide -- what's the signature for the method, what should executables contain, and how is finding done based on those two things? Can you add that?

tgamblin · 2020-05-05T23:54:06Z

lib/spack/docs/build_settings.rst

+* A package must define ``executables`` and ``determine_spec_details``
+  for Spack to locate instances of that package.
+* This is currently intended to find build dependencies rather than
+  library packages.


Probably should say why -- because it can only find executable programs right now.

tgamblin · 2020-05-05T23:54:27Z

lib/spack/docs/build_settings.rst

+  can help locate all external entries).
+* Currently this logic is focused on examining ``PATH`` and does not
+  search through modules (although it should find the package if a
+  module is loaded for it).


I think this is related to the second bullet above and should be combined.

tgamblin

@scheibelp: can you add the documentation above in a follow-on PR?

adamjstewart · 2020-05-16T16:18:46Z

It seems like spack external find can only find the first instance of an external package? What if you have multiple copies of a package like Python installed and you want to pick up both Python 2 and 3?

adamjstewart · 2020-05-16T16:33:27Z

Also, the prefix Spack chooses is non-deterministic. Sometimes I get my Spack-installed Python 3, sometimes I get my system Python 2, and sometimes I get spack-python.

P.S. I'm working on adding spack external find python support

adamjstewart · 2020-05-16T16:38:40Z

Ah, now that I have it working it seems to run this function for each unique prefix. What about when both Python 2 and 3 are installed to the same directory?

alalazo · 2020-05-16T17:10:29Z

What about when both Python 2 and 3 are installed to the same directory?

@adamjstewart You might want to look at #16526 which builds on this PR and implements detection logic for gcc.

adamjstewart · 2020-05-16T17:37:57Z

Also, this PR suffers from the same issue as #12313, it reorganizes my packages.yaml, including existing entries.

adamjstewart · 2020-05-17T22:37:33Z

Btw, I got bored, so I'm going to add spack external find support for common build dependencies, like:

$ egrep -ho 'depends_on\(.(\w+).*type=.build' */package.py | cut -c 13- | egrep -o '^[A-Za-z0-9-]+' | sort | uniq -c | sort -n
...
  38 gmake
  41 flex
  53 bison
  54 py-cython
  61 python
  79 xproto
 155 m4
 181 util-macros
 187 automake
 190 autoconf
 195 libtool
 286 cmake
 347 pkgconfig
 719 py-setuptools

scheibelp added 3 commits February 21, 2020 13:30

incomplete logic for finding exes for packages

d476c0d

minimal work to find cmake executable and print out config for it

ce930d9

implement determine_spec_details for CMake

e8e8c4b

scheibelp added the WIP label Feb 21, 2020

flake edits

0515d8e

adamjstewart added external-packages new-command labels Feb 22, 2020

scheibelp added 4 commits February 27, 2020 17:04

update bash completion script

a71bd99

Package.executables are treated as regular expressions rather than as…

2e396ca

… static strings

sort for repeatability

6cd3f1b

reference error

7ff06fb

scheibelp requested a review from alalazo March 10, 2020 17:23

merge from develop (conflict in CMake package)

980bd72

alalazo reviewed Mar 25, 2020

View reviewed changes

lib/spack/spack/cmd/external.py Show resolved Hide resolved

lib/spack/spack/cmd/external.py Outdated Show resolved Hide resolved

lib/spack/spack/cmd/external.py Outdated Show resolved Hide resolved

lib/spack/spack/cmd/external.py Outdated Show resolved Hide resolved

scheibelp added 10 commits April 20, 2020 13:20

merge from develop (minor conflict in the CMake package)

07ab334

allow specifying executables as input to function to make it easier t…

29f4c00

…o test the function

allow 'spack find external' to detect multiple instances of the same …

5e46bd4

…package (e.g. with different versions)

update tracking of case where the same exe is claimed to be an instan…

5fb829d

…ce of multiple packages

remove useless comment

dc9f034

allow multiple binaries with the same name to refer to different inst…

7380160

…ances of a package (e.g. two instances of CMake stored in different paths)

there was an error when an entry of PATH was not an existing director…

acb139f

…y (the logic was trying to os.listdir on it)

only test files that are executable by the user when considering them…

851e0f3

… as external packages

add docstrings to a couple methods

6339b2e

use spack_yaml config object construction instead of string formattin…

62933e5

…g to create external package configs

This comment has been minimized.

Sign in to view

tgamblin self-requested a review April 21, 2020 03:28

scheibelp added 3 commits April 29, 2020 16:57

avoid recompiling same regex many times

775759e

make test more specific

8c3e8da

formatting issue: space

56821a6

scheibelp removed the WIP label Apr 30, 2020

message that informs users that determine_spec_details decideded a ca…

aed7dd5

…ndidate exe was not an instance of the package was triggering incorrectly. remove confusing and incorrect use of for/else python syntax

alalazo approved these changes May 1, 2020

View reviewed changes

tgamblin requested changes May 5, 2020

View reviewed changes

add docs for 'spack external find'

8af0872

tgamblin approved these changes May 5, 2020

View reviewed changes

tgamblin requested changes May 5, 2020

View reviewed changes

tgamblin approved these changes May 6, 2020

View reviewed changes

tgamblin merged commit b030a81 into spack:develop May 6, 2020

scheibelp added a commit to scheibelp/spack that referenced this pull request May 6, 2020

update docs based on review comments in spack#15158

d2c2bde

scheibelp mentioned this pull request May 6, 2020

Update docs on "spack external find" #16482

Merged

alalazo mentioned this pull request May 18, 2020

Extend packages.yaml to allow custom attributes #16526

Closed

8 tasks

alalazo mentioned this pull request Jun 12, 2020

Generating potential packages.yaml file? #3181

Closed

alexsavulescu mentioned this pull request Jun 18, 2020

Fixes for Neurodamus and Mac instructions BlueBrain/spack#841

Merged

alalazo mentioned this pull request Jun 23, 2020

A more robust way to support external packages #17168

Open

becker33 mentioned this pull request Jul 7, 2020

[idea] external packages via pkg-config #212

Closed

alalazo mentioned this pull request Jul 14, 2020

Move compiler detection logic to packages #17525

Closed

7 tasks

alalazo mentioned this pull request Aug 11, 2020

New format for packages.yaml, simplify detection protocol #17804

Merged

	return Spec('cmake@{0}'.format(version_str))
	return 'cmake@{0}'.format(version_str)

Conversation

scheibelp commented Feb 21, 2020 • edited by alalazo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Mar 11, 2020

Uh oh!

scheibelp commented Mar 21, 2020

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

scheibelp commented Apr 29, 2020

Uh oh!

scheibelp commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo May 1, 2020

Choose a reason for hiding this comment

Uh oh!

scheibelp May 4, 2020

Choose a reason for hiding this comment

Uh oh!

alalazo May 1, 2020

Choose a reason for hiding this comment

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

scheibelp commented May 5, 2020

Uh oh!

tgamblin May 5, 2020

Choose a reason for hiding this comment

Uh oh!

tgamblin May 5, 2020

Choose a reason for hiding this comment

Uh oh!

tgamblin May 5, 2020

Choose a reason for hiding this comment

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented May 16, 2020

Uh oh!

adamjstewart commented May 16, 2020

Uh oh!

adamjstewart commented May 16, 2020

Uh oh!

alalazo commented May 16, 2020

Uh oh!

adamjstewart commented May 16, 2020

Uh oh!

adamjstewart commented May 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

scheibelp commented Feb 21, 2020 •

edited by alalazo

Loading

scheibelp commented Apr 29, 2020 •

edited

Loading