feature : register external packages in the DB by alalazo · Pull Request #1167 · spack/spack

alalazo · 2016-07-05T08:10:35Z

Modifications

external packages are registered in the db and generate module files
added an attribute external to each spec in the DB entry
added unit tests to stress the new feature

alalazo · 2016-07-05T11:14:52Z

@tgamblin @becker33 @mathstuf @mplegendre I think this is ready to be reviewed

mathstuf · 2016-07-18T16:35:46Z

lib/spack/spack/database.py

        d = dictionary
        return InstallRecord(spec, d['path'], d['installed'], d['ref_count'],
-                             d.get('explicit', False))
+                             d['explicit'], d['external'])


Hmm. Seems this would be best as **dictionary. Does that work?

@mathstuf Apparently not... spec may be in dictionary, so I had to resort to popping it out before doing a **d. Would this be good enough ?

tgamblin · 2016-10-31T21:47:23Z

@alalazo: this looks good to me except for one thing. I think the spec should be where things are stored as external or not. If I build a package with one version of an external package, and then I change the external location, that should be considered a different build and a different package, and it should change the hash. Would you agree with that?

Places where this would help:

This would let you regenerate almost the complete state of the DB. In the current implementation, reindex would not do this b/c the external info is not stored in spec.yaml.
We would need this information if we're going to use spec.yaml descriptors to identify binary packages. There should be a difference between a package installed with a spack-built version of a spec and one installed with a non-spack-built version.
If we don't do this, you could get a hash conflict in the DB if you first build a package with a Spack MPI, then build it with an external MPI.

Right now the Spec has an external field, but it is only maintained from concretization time to install time. It's not actually read in from the DB. I was thinking it would be worth adding, to external specs, a field like this:

external:
    path: /path/to/package
    module: name-of-module

Either path or module could be specified in the spec.

What do you think?

alalazo · 2016-11-01T11:47:48Z

lib/spack/spack/package.py

    def installed(self):
-        return os.path.isdir(self.prefix)
+        try:
+            rec = spack.installed_db.get_record(self.spec)


@tgamblin If I remember well this line was slowing things down. Maybe the situation won't be that bad after JSON indexing though...

alalazo · 2016-11-01T11:49:57Z

@tgamblin Do you want this for v0.10 ?

alalazo · 2016-11-01T11:50:52Z

What do you think?

It seems a good plan to me

tgamblin · 2016-11-01T11:51:35Z

Depends on how long it would take. If you can do it this week I think it is reasonable to put in. Imho it's how this should work... what do you think?

alalazo · 2016-11-01T11:52:45Z

Depends on how long it would take. If you can do it this week I think it is reasonable to put in. Imho it's how this should work... what do you think?

Fine with me. I'll try to do this asap.

alalazo · 2017-01-13T17:58:46Z

@tgamblin Just to let you know: I restarted working on this. The current version should work as you asked, but I didn't have the possibility to check it on Cray (for the external_module part). I still need to do polish a bit the code and add tests. Will ping you when done.

alalazo · 2017-01-15T10:20:26Z

@mplegendre @tgamblin PR ready for review, unit tests have been added.

adamjstewart · 2017-01-15T13:23:02Z

@tgamblin A thought I had on changing hashes. Since Spack is no longer compatible with installations done under a previous hashing mechanism, users are forced to either uninstall everything or create a new Spack installation. Could we increment the version number of Spack every time this happens? Instead of incrementing the version during each release, the version of develop should be incremented to reflect changes in the hashing mechanism.

alalazo · 2017-01-15T15:11:50Z

@adamjstewart Just my 2 cents: I don't like very much this idea. I think that as the core of Spack becomes more stable we should try to go for time-based releases (e.g. every n months), increment the version number on releases and feature-freeze some time before a new release to focus only on bug-fixes and documentation.

develop shouldn't be treated as a moving stable release because doing so will either prevent experimenting with new design (as we need to be conservative to maintain stability) or it will compromise stability (giving users the impression that the tool is not ready for production). Besides we will be already at Spack 5.0 which would be weird 😄

adamjstewart · 2017-01-15T19:33:05Z

I agree, and this makes perfect sense for a regular project. But I currently have a spack-0.9.1 installation, and a spack-0.10 installation from the last time the hash changed. Once this is merged, I'll have a spack-0.10.1 installation, but the real core Spack will still be on 0.9.1. We either need to fix Spack so that a change in the hashing algorithm doesn't require you to reinstall it, or we need some way to make it clear that "Spack after this PR is merged" is incompatible with "Spack before this PR is merged". Incrementing the version number is at least one way of indicating this.

…f a spec/yaml round-trip for things that involve an external

* Improve implementation of external packages * Don't find external module path at each step of concretization it's not necessary.. The paths are retrieved at the end of concretizaion * Don't find replacements for external packages. * Test root of the DAG if external No reason not to test if the root of the DAG is external when external packages are now first class citizens! * Clean up statements in package for external modules * Correct serious flaw with external package code I made earlier * Create external property for Spec. * Remove some stray externals * Allow users to specify external package paths relative to spack Canonicalize external package paths so that users may specify their locations relative to spack's directory. * Remove leftover external= statement * Update tests to use new external_path and external properly.

…s on external

alalazo · 2017-04-22T09:20:04Z

Tested manually with this packages.yaml:

packages:
  zlib:
    buildable:	False
    paths:
      [email protected]%[email protected]: /usr/lib/x86_64-linux-gnu
  openssl:
    buildable:	False
    paths:
      openssl@system%[email protected]: /usr/lib/x86_64-linux-gnu

The result is the following:

$ spack install openssl
==> openssl@system : externally installed in /usr/lib/x86_64-linux-gnu
==> openssl@system : generating module file
==> openssl@system : registering into DB

$ spack install hdf5~mpi+szip
==> [email protected] : externally installed in /usr/lib/x86_64-linux-gnu
==> [email protected] : generating module file
==> [email protected] : registering into DB
==> Installing szip
==> Using cached archive: /home/mculpo/PycharmProjects/spack/var/spack/cache/szip/szip-2.1.tar.gz
==> Staging archive: /home/mculpo/PycharmProjects/spack/var/spack/stage/szip-2.1-uxcvrxb4jpm3rkqjwmburqcaobtac6mw/szip-2.1.tar.gz
==> Created stage in /home/mculpo/PycharmProjects/spack/var/spack/stage/szip-2.1-uxcvrxb4jpm3rkqjwmburqcaobtac6mw
==> Ran patch() for szip
==> Building szip [AutotoolsPackage]
==> Executing phase : 'autoreconf'
==> Executing phase : 'configure'
==> Executing phase : 'build'
==> Executing phase : 'install'
==> Successfully installed szip
  Fetch: 0.00s.  Build: 8.60s.  Total: 8.60s.
[+] /home/mculpo/PycharmProjects/spack/opt/spack/linux-ubuntu14-x86_64/gcc-4.8/szip-2.1-uxcvrxb4jpm3rkqjwmburqcaobtac6mw
==> Installing hdf5
==> Using cached archive: /home/mculpo/PycharmProjects/spack/var/spack/cache/hdf5/hdf5-1.10.0-patch1.tar.gz
==> Staging archive: /home/mculpo/PycharmProjects/spack/var/spack/stage/hdf5-1.10.0-patch1-slyc4764jhitnou4jpaf4lo5l3qvovdp/hdf5-1.10.0-patch1.tar.gz
==> Created stage in /home/mculpo/PycharmProjects/spack/var/spack/stage/hdf5-1.10.0-patch1-slyc4764jhitnou4jpaf4lo5l3qvovdp
==> Ran patch() for hdf5
==> Building hdf5 [AutotoolsPackage]
==> Executing phase : 'autoreconf'
==> Executing phase : 'configure'
==> Executing phase : 'build'
==> Executing phase : 'install'
==> Successfully installed hdf5
  Fetch: 0.04s.  Build: 6m 24.44s.  Total: 6m 24.48s.
[+] /home/mculpo/PycharmProjects/spack/opt/spack/linux-ubuntu14-x86_64/gcc-4.8/hdf5-1.10.0-patch1-slyc4764jhitnou4jpaf4lo5l3qvovdp

$ cp -r opt/spack/.spack-db/ ~/tmp/

$ spack -d reindex
==> WRITE LOCK: /home/mculpo/PycharmProjects/spack/opt/spack/.spack-db/lock[0:0] [Acquiring]
==> RECONSTRUCTING FROM SPEC.YAML: [email protected]%[email protected] arch=linux-ubuntu14-x86_64
==> RECONSTRUCTING FROM SPEC.YAML: [email protected]%[email protected]+cxx~debug+fortran~mpi+pic+shared+szip~threadsafe arch=linux-ubuntu14-x86_64 ^[email protected]%[email protected] arch=linux-ubuntu14-x86_64 ^[email protected]%[email protected]+pic+shared arch=linux-ubuntu14-x86_64
==> RECONSTRUCTING FROM OLD DB: [email protected]%[email protected]+pic+shared arch=linux-ubuntu14-x86_64
==> SKIPPING RECONSTRUCTION FROM OLD DB: [email protected]%[email protected] arch=linux-ubuntu14-x86_64 [already reconstructed from spec.yaml]
==> RECONSTRUCTING FROM OLD DB: openssl@system%[email protected] arch=linux-ubuntu14-x86_64
==> SKIPPING RECONSTRUCTION FROM OLD DB: [email protected]%[email protected]+cxx~debug+fortran~mpi+pic+shared+szip~threadsafe arch=linux-ubuntu14-x86_64 ^[email protected]%[email protected] arch=linux-ubuntu14-x86_64 ^[email protected]%[email protected]+pic+shared arch=linux-ubuntu14-x86_64 [already reconstructed from spec.yaml]
==> WRITE LOCK: /home/mculpo/PycharmProjects/spack/opt/spack/.spack-db/lock[0:0] [Released]

$ diff opt/spack/.spack-db/index.json ~/tmp/.spack-db/index.json

alalazo · 2017-04-22T09:24:42Z

Is this an artifact of the way this PR started -- with external stored only in the DB and not in the spec? Could we just keep the old logic?

I added the logic to take care of the fact that you may have registered external packages that are not dependencies of anything else (think of an externally installed Intel compiler, or an external licensed application), so that they could be reconstructed as well. I agree that the logic should be reversed, and this is fixed now.

I also added comments in the code to not lose memory of what we said here. Let me kow if I need to rephrase anything to make it clearer.

alalazo · 2017-04-22T09:35:36Z

Ok, this should be ready for another review round.

tgamblin

LGTM! Minor comments but finally done. Thanks for all the updates!

tgamblin · 2017-04-23T00:48:30Z

lib/spack/spack/database.py

+            # considered authoritative with respect to DB reindexing, as
+            # entries in the DB may be corrupted in a way that still makes
+            # them readable. If we considered DB entries authoritative
+            # instead, we would perpetuate errors over a reindex.


We probably should add an option to propagate only specs from spec.yaml files, but that can wait for a separate PR. I don't think there is an immediate need for it.

tgamblin · 2017-04-23T01:00:53Z

lib/spack/spack/database.py

+                    except Exception as e:
+                        # Something went wrong, so the spec was not restored
+                        # from old data
+                        tty.debug(e.message)


Also for separate PR: it would be good to add something like:

if spack.debug: print(traceback.format_exc())

So we don't lose the trace.

tgamblin · 2017-04-23T01:02:58Z

lib/spack/spack/package.py

+            tty.msg(message.format(s=self, module=self.spec.external_module))
+            message = '{s.name}@{s.version} : is actually installed in {path}'
+            tty.msg(message.format(s=self, path=self.spec.external_path))
+        else:


generally good style to either assert self.spec.external or to say eilf self.spec.external_path: ... else: raise ValueError("spec was not external")

Fixes spack#4026 spack#1167 updated Database.reindex to keep old installation records to support external packages. However, when a user manually removes a prefix and reindexes this kept the records so the packages were still installed according to "spack find" etc. This adds a check for non-external packages to ensure they are properly installed according to the directory layout.

Fixes #4026 #1167 updated Database.reindex to keep old installation records to support external packages. However, when a user manually removes a prefix and reindexes this kept the records so the packages were still installed according to "spack find" etc. This adds a check for non-external packages to ensure they are properly installed according to the directory layout.

@tgamblin

* treats correctly a change from `explicit=False` to `explicit=True` in an external package DB entry. * added unit tests * fixed issues raised by @tgamblin . In particular the PR is no more hash-changing for packages that are not external. * added a test to check correctness of a spec/yaml round-trip for things that involve an external * Don't find external module path at each step of concretization * it's not necessary.. The paths are retrieved at the end of concretizaion * Don't find replacements for external packages. * Test root of the DAG if external * No reason not to test if the root of the DAG is external when external packages are now first class citizens! * Create `external` property for Spec (for external_path and external_module) * Allow users to specify external package paths relative to spack * Canonicalize external package paths so that users may specify their locations relative to spack's directory. * Update tests to use new external_path and external properly. * skip license hooks on external

Fixes spack#4026 spack#1167 updated Database.reindex to keep old installation records to support external packages. However, when a user manually removes a prefix and reindexes this kept the records so the packages were still installed according to "spack find" etc. This adds a check for non-external packages to ensure they are properly installed according to the directory layout.

@tgamblin

* treats correctly a change from `explicit=False` to `explicit=True` in an external package DB entry. * added unit tests * fixed issues raised by @tgamblin . In particular the PR is no more hash-changing for packages that are not external. * added a test to check correctness of a spec/yaml round-trip for things that involve an external * Don't find external module path at each step of concretization * it's not necessary.. The paths are retrieved at the end of concretizaion * Don't find replacements for external packages. * Test root of the DAG if external * No reason not to test if the root of the DAG is external when external packages are now first class citizens! * Create `external` property for Spec (for external_path and external_module) * Allow users to specify external package paths relative to spack * Canonicalize external package paths so that users may specify their locations relative to spack's directory. * Update tests to use new external_path and external properly. * skip license hooks on external

Fixes spack#4026 spack#1167 updated Database.reindex to keep old installation records to support external packages. However, when a user manually removes a prefix and reindexes this kept the records so the packages were still installed according to "spack find" etc. This adds a check for non-external packages to ensure they are properly installed according to the directory layout.

@tgamblin

* treats correctly a change from `explicit=False` to `explicit=True` in an external package DB entry. * added unit tests * fixed issues raised by @tgamblin . In particular the PR is no more hash-changing for packages that are not external. * added a test to check correctness of a spec/yaml round-trip for things that involve an external * Don't find external module path at each step of concretization * it's not necessary.. The paths are retrieved at the end of concretizaion * Don't find replacements for external packages. * Test root of the DAG if external * No reason not to test if the root of the DAG is external when external packages are now first class citizens! * Create `external` property for Spec (for external_path and external_module) * Allow users to specify external package paths relative to spack * Canonicalize external package paths so that users may specify their locations relative to spack's directory. * Update tests to use new external_path and external properly. * skip license hooks on external

Fixes spack#4026 spack#1167 updated Database.reindex to keep old installation records to support external packages. However, when a user manually removes a prefix and reindexes this kept the records so the packages were still installed according to "spack find" etc. This adds a check for non-external packages to ensure they are properly installed according to the directory layout.

@tgamblin

* treats correctly a change from `explicit=False` to `explicit=True` in an external package DB entry. * added unit tests * fixed issues raised by @tgamblin . In particular the PR is no more hash-changing for packages that are not external. * added a test to check correctness of a spec/yaml round-trip for things that involve an external * Don't find external module path at each step of concretization * it's not necessary.. The paths are retrieved at the end of concretizaion * Don't find replacements for external packages. * Test root of the DAG if external * No reason not to test if the root of the DAG is external when external packages are now first class citizens! * Create `external` property for Spec (for external_path and external_module) * Allow users to specify external package paths relative to spack * Canonicalize external package paths so that users may specify their locations relative to spack's directory. * Update tests to use new external_path and external properly. * skip license hooks on external

* Updates the CoreNEURON package to be consistent with the CI * Clean references and configurations to old versions

alalazo changed the title ~~feature : register external packages in the DB~~ [WIP] feature : register external packages in the DB Jul 5, 2016

alalazo changed the title ~~[WIP] feature : register external packages in the DB~~ feature : register external packages in the DB Jul 5, 2016

mathstuf reviewed Jul 18, 2016
View reviewed changes

alalazo mentioned this pull request Jul 21, 2016

OpenSSL checks are too slow #1332

Closed

davydden added feature A feature is missing in Spack ready labels Sep 21, 2016

alalazo commented Nov 1, 2016

View reviewed changes

alalazo added WIP and removed ready labels Nov 30, 2016

alalazo mentioned this pull request Dec 10, 2016

External package support is generally broken #2550

Closed

alalazo force-pushed the features/external_packages_are_first_class_citizens branch from b272410 to fddb921 Compare January 13, 2017 17:54

alalazo self-assigned this Jan 13, 2017

alalazo added the hash-change label Jan 13, 2017

alalazo force-pushed the features/external_packages_are_first_class_citizens branch from 8dcc62c to 15c6ecd Compare January 14, 2017 11:10

alalazo added ready and removed WIP labels Jan 15, 2017

alalazo requested review from mplegendre and tgamblin January 15, 2017 10:19

alalazo and others added 5 commits April 22, 2017 09:52

register_external_packages_in_db: added a test to check correctness o…

437690f

…f a spec/yaml round-trip for things that involve an external

qa: flake8 F401 in package_prefs.py

f40d663

DB: fixed issues due to rebase

a293012

fixed issues raised by @tgamblin during review, skipping license hook…

5af97a7

…s on external

alalazo force-pushed the features/external_packages_are_first_class_citizens branch from 3f0760e to 5af97a7 Compare April 22, 2017 09:18

tgamblin approved these changes Apr 23, 2017

View reviewed changes

tgamblin merged commit 3b52d0a into spack:develop Apr 23, 2017

alalazo deleted the features/external_packages_are_first_class_citizens branch April 23, 2017 07:40

tgamblin mentioned this pull request Apr 25, 2017

Override partial installs by default - part two #3929

Closed

adamjstewart mentioned this pull request Apr 27, 2017

Don't print successfully uninstalled twice #4019

Merged

scheibelp mentioned this pull request Apr 27, 2017

Uninstalled packages in the database #4026

Closed

scheibelp mentioned this pull request Apr 27, 2017

Reindex checks install for non-external packages #4027

Merged

alalazo mentioned this pull request May 2, 2017

should spack find return info regarding packages in packages.yaml #4075

Closed

alalazo mentioned this pull request May 12, 2017

Lmod and changing hashes #4233

Closed

tgamblin added this to the v0.11.0 milestone Nov 12, 2017

alalazo mentioned this pull request May 14, 2018

Package.installed now checks both the presence of prefix and a DB entry #8038

Merged

olupton pushed a commit to olupton/spack that referenced this pull request Feb 7, 2022

Update CoreNEURON package to be consistent with the CI (spack#1167)

bad8560

* Updates the CoreNEURON package to be consistent with the CI * Clean references and configurations to old versions

Conversation

alalazo commented Jul 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Modifications

Uh oh!

alalazo commented Jul 5, 2016

Uh oh!

mathstuf Jul 18, 2016

Choose a reason for hiding this comment

Uh oh!

alalazo Jul 19, 2016

Choose a reason for hiding this comment

Uh oh!

tgamblin commented Oct 31, 2016

Uh oh!

alalazo Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

alalazo commented Nov 1, 2016

Uh oh!

alalazo commented Nov 1, 2016

Uh oh!

tgamblin commented Nov 1, 2016

Uh oh!

alalazo commented Nov 1, 2016

Uh oh!

alalazo commented Jan 13, 2017

Uh oh!

alalazo commented Jan 15, 2017

Uh oh!

adamjstewart commented Jan 15, 2017

Uh oh!

alalazo commented Jan 15, 2017

Uh oh!

adamjstewart commented Jan 15, 2017

Uh oh!

alalazo commented Apr 22, 2017

Uh oh!

alalazo commented Apr 22, 2017

Uh oh!

alalazo commented Apr 22, 2017

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

tgamblin Apr 23, 2017

Choose a reason for hiding this comment

Uh oh!

tgamblin Apr 23, 2017

Choose a reason for hiding this comment

Uh oh!

tgamblin Apr 23, 2017

Choose a reason for hiding this comment

Uh oh!

tgamblin Apr 23, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alalazo commented Jul 5, 2016 •

edited

Loading