zhangyiss/spack
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix: hash-based references to upstream specs (#14629)

Browse Source 
Spack commands referring to upstream-installed specs by hash have
been broken since 6b619da (merged September 2019), which added a new
Database function specifically for parsing hashes from command-line
specs; this function was inappropriately attempting to acquire locks
on upstream databases.

This PR updates the offending function to avoid locking upstream
databases and also updates associated tests to catch regression
errors: the upstream database created for these tests was not
explicitly set as an upstream (i.e. initialized with upstream=True)
so it was not guarding against inappropriate accesses.
This commit is contained in:
Andrew W Elble 2020-01-27 21:25:23 -05:00 committed by Peter Scheibel
parent 7badd69d1e
commit d86816bc1a
2 changed files with 56 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

52
lib/spack/spack/database.py
View File

@ -1118,7 +1118,27 @@ def activated_extensions_for(self, extendee_spec, extensions_layout=None):
continue
# TODO: conditional way to do this instead of catching exceptions
def get_by_hash_local(self, dag_hash, default=None, installed=any):
def _get_by_hash_local(self, dag_hash, default=None, installed=any):
# hash is a full hash and is in the data somewhere
if dag_hash in self._data:
rec = self._data[dag_hash]
if rec.install_type_matches(installed):
return [rec.spec]
else:
return default
# check if hash is a prefix of some installed (or previously
# installed) spec.
matches = [record.spec for h, record in self._data.items()
if h.startswith(dag_hash) and
record.install_type_matches(installed)]
if matches:
return matches
# nothing found
return default
def get_by_hash_local(self, *args, **kwargs):
"""Look up a spec in *this DB* by DAG hash, or by a DAG hash prefix.
Arguments:
@ -1142,24 +1162,7 @@ def get_by_hash_local(self, dag_hash, default=None, installed=any):
"""
with self.read_transaction():
# hash is a full hash and is in the data somewhere
if dag_hash in self._data:
rec = self._data[dag_hash]
if rec.install_type_matches(installed):
return [rec.spec]
else:
return default
# check if hash is a prefix of some installed (or previously
# installed) spec.
matches = [record.spec for h, record in self._data.items()
if h.startswith(dag_hash) and
record.install_type_matches(installed)]
if matches:
return matches
# nothing found
return default
return self._get_by_hash_local(*args, **kwargs)
def get_by_hash(self, dag_hash, default=None, installed=any):
"""Look up a spec by DAG hash, or by a DAG hash prefix.
@ -1184,9 +1187,14 @@ def get_by_hash(self, dag_hash, default=None, installed=any):
(list): a list of specs matching the hash or hash prefix
"""
search_path = [self] + self.upstream_dbs
for db in search_path:
spec = db.get_by_hash_local(
spec = self.get_by_hash_local(
dag_hash, default=default, installed=installed)
if spec is not None:
return spec
for upstream_db in self.upstream_dbs:
spec = upstream_db._get_by_hash_local(
dag_hash, default=default, installed=installed)
if spec is not None:
return spec

40
lib/spack/spack/test/database.py
View File

@ -41,10 +41,11 @@ def test_store(tmpdir):
@pytest.fixture()
def upstream_and_downstream_db(tmpdir_factory, gen_mock_layout):
mock_db_root = str(tmpdir_factory.mktemp('mock_db_root'))
upstream_db = spack.database.Database(mock_db_root)
upstream_write_db = spack.database.Database(mock_db_root)
upstream_db = spack.database.Database(mock_db_root, is_upstream=True)
# Generate initial DB file to avoid reindex
with open(upstream_db._index_path, 'w') as db_file:
upstream_db._write_to_file(db_file)
with open(upstream_write_db._index_path, 'w') as db_file:
upstream_write_db._write_to_file(db_file)
upstream_layout = gen_mock_layout('/a/')
downstream_db_root = str(
@ -55,13 +56,14 @@ def upstream_and_downstream_db(tmpdir_factory, gen_mock_layout):
downstream_db._write_to_file(db_file)
downstream_layout = gen_mock_layout('/b/')
yield upstream_db, upstream_layout, downstream_db, downstream_layout
yield upstream_write_db, upstream_db, upstream_layout,\
downstream_db, downstream_layout
@pytest.mark.usefixtures('config')
def test_installed_upstream(upstream_and_downstream_db):
upstream_db, upstream_layout, downstream_db, downstream_layout = (
upstream_and_downstream_db)
upstream_write_db, upstream_db, upstream_layout,\
downstream_db, downstream_layout = (upstream_and_downstream_db)
default = ('build', 'link')
x = MockPackage('x', [], [])
@ -75,7 +77,14 @@ def test_installed_upstream(upstream_and_downstream_db):
spec.concretize()
for dep in spec.traverse(root=False):
upstream_db.add(dep, upstream_layout)
upstream_write_db.add(dep, upstream_layout)
upstream_db._read()
for dep in spec.traverse(root=False):
record = downstream_db.get_by_hash(dep.dag_hash())
assert record is not None
with pytest.raises(spack.database.ForbiddenLockError):
record = upstream_db.get_by_hash(dep.dag_hash())
new_spec = spack.spec.Spec('w')
new_spec.concretize()
@ -96,8 +105,8 @@ def test_installed_upstream(upstream_and_downstream_db):
@pytest.mark.usefixtures('config')
def test_removed_upstream_dep(upstream_and_downstream_db):
upstream_db, upstream_layout, downstream_db, downstream_layout = (
upstream_and_downstream_db)
upstream_write_db, upstream_db, upstream_layout,\
downstream_db, downstream_layout = (upstream_and_downstream_db)
default = ('build', 'link')
z = MockPackage('z', [], [])
@ -108,13 +117,15 @@ def test_removed_upstream_dep(upstream_and_downstream_db):
spec = spack.spec.Spec('y')
spec.concretize()
upstream_db.add(spec['z'], upstream_layout)
upstream_write_db.add(spec['z'], upstream_layout)
upstream_db._read()
new_spec = spack.spec.Spec('y')
new_spec.concretize()
downstream_db.add(new_spec, downstream_layout)
upstream_db.remove(new_spec['z'])
upstream_write_db.remove(new_spec['z'])
upstream_db._read()
new_downstream = spack.database.Database(
downstream_db.root, upstream_dbs=[upstream_db])
@ -129,8 +140,8 @@ def test_add_to_upstream_after_downstream(upstream_and_downstream_db):
DB. When a package is recorded as installed in both, the results should
refer to the downstream DB.
"""
upstream_db, upstream_layout, downstream_db, downstream_layout = (
upstream_and_downstream_db)
upstream_write_db, upstream_db, upstream_layout,\
downstream_db, downstream_layout = (upstream_and_downstream_db)
x = MockPackage('x', [], [])
mock_repo = MockPackageMultiRepo([x])
@ -141,7 +152,8 @@ def test_add_to_upstream_after_downstream(upstream_and_downstream_db):
downstream_db.add(spec, downstream_layout)
upstream_db.add(spec, upstream_layout)
upstream_write_db.add(spec, upstream_layout)
upstream_db._read()
upstream, record = downstream_db.query_by_spec_hash(spec.dag_hash())
# Even though the package is recorded as installed in the upstream DB,