-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dependency types #378
dependency types #378
Changes from all commits
e275b56
b4682c8
6d2ec9b
1ae43b3
bdf8224
bae97d1
45c675f
6fd4552
77965ce
ac1701a
0ca1a48
a82385c
39aef5f
d71a124
a0584c7
faa3d43
5d152ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,9 +215,14 @@ def _read_spec_from_yaml(self, hash_key, installs, parent_key=None): | |
# Add dependencies from other records in the install DB to | ||
# form a full spec. | ||
if 'dependencies' in spec_dict[spec.name]: | ||
for dep_hash in spec_dict[spec.name]['dependencies'].values(): | ||
for dep in spec_dict[spec.name]['dependencies'].values(): | ||
if type(dep) == tuple: | ||
dep_hash, deptypes = dep | ||
else: | ||
dep_hash = dep | ||
deptypes = spack.alldeps | ||
child = self._read_spec_from_yaml(dep_hash, installs, hash_key) | ||
spec._add_dependency(child) | ||
spec._add_dependency(child, deptypes) | ||
|
||
# Specs from the database need to be marked concrete because | ||
# they represent actual installations. | ||
|
@@ -334,7 +339,10 @@ def _check_ref_counts(self): | |
counts = {} | ||
for key, rec in self._data.items(): | ||
counts.setdefault(key, 0) | ||
for dep in rec.spec.dependencies.values(): | ||
# XXX(deptype): This checks all dependencies, but build | ||
# dependencies might be able to be dropped in the | ||
# future. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this consistent with the ref counting? It looks like you only increment for link here, but this checks for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dependency queries get all by default. Only traversal defaults to |
||
for dep in rec.spec.dependencies(): | ||
dep_key = dep.dag_hash() | ||
counts.setdefault(dep_key, 0) | ||
counts[dep_key] += 1 | ||
|
@@ -406,7 +414,7 @@ def _add(self, spec, path, directory_layout=None, explicit=False): | |
else: | ||
self._data[key] = InstallRecord(spec, path, True, | ||
explicit=explicit) | ||
for dep in spec.dependencies.values(): | ||
for dep in spec.dependencies(('link', 'run')): | ||
self._increment_ref_count(dep, directory_layout) | ||
|
||
def _increment_ref_count(self, spec, directory_layout=None): | ||
|
@@ -421,7 +429,7 @@ def _increment_ref_count(self, spec, directory_layout=None): | |
|
||
self._data[key] = InstallRecord(spec.copy(), path, installed) | ||
|
||
for dep in spec.dependencies.values(): | ||
for dep in spec.dependencies('link'): | ||
self._increment_ref_count(dep) | ||
|
||
self._data[key].ref_count += 1 | ||
|
@@ -466,7 +474,7 @@ def _decrement_ref_count(self, spec): | |
|
||
if rec.ref_count == 0 and not rec.installed: | ||
del self._data[key] | ||
for dep in spec.dependencies.values(): | ||
for dep in spec.dependencies('link'): | ||
self._decrement_ref_count(dep) | ||
|
||
def _remove(self, spec): | ||
|
@@ -480,7 +488,7 @@ def _remove(self, spec): | |
return rec.spec | ||
|
||
del self._data[key] | ||
for dep in rec.spec.dependencies.values(): | ||
for dep in rec.spec.dependencies('link'): | ||
self._decrement_ref_count(dep) | ||
|
||
# Returns the concrete spec so we know it in the case where a | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -171,7 +171,7 @@ def version(pkg, ver, checksum=None, **kwargs): | |
pkg.versions[Version(ver)] = kwargs | ||
|
||
|
||
def _depends_on(pkg, spec, when=None): | ||
def _depends_on(pkg, spec, when=None, type=None): | ||
# If when is False do nothing | ||
if when is False: | ||
return | ||
|
@@ -180,24 +180,43 @@ def _depends_on(pkg, spec, when=None): | |
when = pkg.name | ||
when_spec = parse_anonymous_spec(when, pkg.name) | ||
|
||
if type is None: | ||
# The default deptype is build and link because the common case is to | ||
# build against a library which then turns into a runtime dependency | ||
# due to the linker. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like an outdated comment. From the description of i suppose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not, actually. The package itself does not call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think for distant observer like me it is not clear what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An easier way to think about this is whether, when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I am not mistaken There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. To understand this you have to distinguish runtime from build time. |
||
# XXX(deptype): Add 'run' to this? It's an uncommon dependency type, | ||
# but is most backwards-compatible. | ||
type = ('build', 'link') | ||
|
||
if isinstance(type, str): | ||
type = (type,) | ||
|
||
for deptype in type: | ||
if deptype not in spack.spec.alldeps: | ||
raise UnknownDependencyTypeError('depends_on', pkg.name, deptype) | ||
|
||
dep_spec = Spec(spec) | ||
if pkg.name == dep_spec.name: | ||
raise CircularReferenceError('depends_on', pkg.name) | ||
|
||
pkg_deptypes = pkg._deptypes.setdefault(dep_spec.name, set()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like I doubt this is an actual, measurable performance problem but the tools guy in me looks at it and winces. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The if 'foo' not in d:
d['foo'] = local = Default()
else:
local = d['foo'] It would be nice if dictionaries had |
||
for deptype in type: | ||
pkg_deptypes.add(deptype) | ||
|
||
conditions = pkg.dependencies.setdefault(dep_spec.name, {}) | ||
if when_spec in conditions: | ||
conditions[when_spec].constrain(dep_spec, deps=False) | ||
else: | ||
conditions[when_spec] = dep_spec | ||
|
||
|
||
@directive('dependencies') | ||
def depends_on(pkg, spec, when=None): | ||
@directive(('dependencies', '_deptypes')) | ||
def depends_on(pkg, spec, when=None, type=None): | ||
"""Creates a dict of deps with specs defining when they apply.""" | ||
_depends_on(pkg, spec, when=when) | ||
_depends_on(pkg, spec, when=when, type=type) | ||
|
||
|
||
@directive(('extendees', 'dependencies')) | ||
@directive(('extendees', 'dependencies', '_deptypes')) | ||
def extends(pkg, spec, **kwargs): | ||
"""Same as depends_on, but dependency is symlinked into parent prefix. | ||
|
||
|
@@ -326,3 +345,13 @@ def __init__(self, directive, package): | |
directive, | ||
"Package '%s' cannot pass itself to %s" % (package, directive)) | ||
self.package = package | ||
|
||
|
||
class UnknownDependencyTypeError(DirectiveError): | ||
"""This is raised when a dependency is of an unknown type.""" | ||
def __init__(self, directive, package, deptype): | ||
super(UnknownDependencyTypeError, self).__init__( | ||
directive, | ||
"Package '%s' cannot depend on a package via %s." % | ||
(package, deptype)) | ||
self.package = package |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,12 +80,14 @@ def topological_sort(spec, **kwargs): | |
|
||
""" | ||
reverse = kwargs.get('reverse', False) | ||
# XXX(deptype): iterate over a certain kind of dependency. Maybe color | ||
# edges based on the type of dependency? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds cool. But don't worry too much about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, any of these |
||
if not reverse: | ||
parents = lambda s: s.dependents | ||
children = lambda s: s.dependencies | ||
parents = lambda s: s.dependents() | ||
children = lambda s: s.dependencies() | ||
else: | ||
parents = lambda s: s.dependencies | ||
children = lambda s: s.dependents | ||
parents = lambda s: s.dependencies() | ||
children = lambda s: s.dependents() | ||
|
||
# Work on a copy so this is nondestructive. | ||
spec = spec.copy() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this supposed to get fixed? not sure what it's for