directives: depends_on should not admit anonymous specs (#34368)
Writing a long dependency like:
```python
     depends_on(
         "llvm"
         "targets=amdgpu,bpf,nvptx,webassembly"
         "version_suffix=jl +link_llvm_dylib ~internal_unwind"
     )
```
when it should be formatted like this:
```python
     depends_on(
         "llvm"
         " targets=amdgpu,bpf,nvptx,webassembly"
         " version_suffix=jl +link_llvm_dylib ~internal_unwind"
     )
```
can cause really subtle errors. Specifically, you'll get something like this in
the package sanity tests:
```
    AttributeError: 'NoneType' object has no attribute 'rpartition'
```
because Spack happily constructs a class that has a dependency with name `None`.
We can catch this earlier by banning anonymous dependency specs directly in
`depends_on()`.  This causes the package itself to fail to parse, and emits
a much better error message:
```
==> Error: Invalid dependency specification in package 'julia':
    llvmtargets=amdgpu,bpf,nvptx,webassemblyversion_suffix=jl +link_llvm_dylib ~internal_unwind
```
			
			
This commit is contained in:
		| @@ -361,6 +361,8 @@ def _depends_on(pkg, spec, when=None, type=default_deptype, patches=None): | |||||||
|         return |         return | ||||||
| 
 | 
 | ||||||
|     dep_spec = spack.spec.Spec(spec) |     dep_spec = spack.spec.Spec(spec) | ||||||
|  |     if not dep_spec.name: | ||||||
|  |         raise DependencyError("Invalid dependency specification in package '%s':" % pkg.name, spec) | ||||||
|     if pkg.name == dep_spec.name: |     if pkg.name == dep_spec.name: | ||||||
|         raise CircularReferenceError("Package '%s' cannot depend on itself." % pkg.name) |         raise CircularReferenceError("Package '%s' cannot depend on itself." % pkg.name) | ||||||
| 
 | 
 | ||||||
| @@ -769,7 +771,11 @@ class DirectiveError(spack.error.SpackError): | |||||||
|     """This is raised when something is wrong with a package directive.""" |     """This is raised when something is wrong with a package directive.""" | ||||||
| 
 | 
 | ||||||
| 
 | 
 | ||||||
| class CircularReferenceError(DirectiveError): | class DependencyError(DirectiveError): | ||||||
|  |     """This is raised when a dependency specification is invalid.""" | ||||||
|  | 
 | ||||||
|  | 
 | ||||||
|  | class CircularReferenceError(DependencyError): | ||||||
|     """This is raised when something depends on itself.""" |     """This is raised when something depends on itself.""" | ||||||
| 
 | 
 | ||||||
| 
 | 
 | ||||||
|   | |||||||
| @@ -4,6 +4,7 @@ | |||||||
| # SPDX-License-Identifier: (Apache-2.0 OR MIT) | # SPDX-License-Identifier: (Apache-2.0 OR MIT) | ||||||
| import pytest | import pytest | ||||||
| 
 | 
 | ||||||
|  | import spack.directives | ||||||
| import spack.repo | import spack.repo | ||||||
| import spack.spec | import spack.spec | ||||||
| 
 | 
 | ||||||
| @@ -60,3 +61,10 @@ def test_extends_spec(config, mock_packages): | |||||||
| 
 | 
 | ||||||
|     assert extender.dependencies |     assert extender.dependencies | ||||||
|     assert extender.package.extends(extendee) |     assert extender.package.extends(extendee) | ||||||
|  | 
 | ||||||
|  | 
 | ||||||
|  | @pytest.mark.regression("34368") | ||||||
|  | def test_error_on_anonymous_dependency(config, mock_packages): | ||||||
|  |     pkg = spack.repo.path.get_pkg_class("a") | ||||||
|  |     with pytest.raises(spack.directives.DependencyError): | ||||||
|  |         spack.directives._depends_on(pkg, "@4.5") | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user
	 Todd Gamblin
					Todd Gamblin