In the Windows filesystem logic for creating a symlink, we intend to
fall back to a copy when the symlink cannot be created (for some
configuration settings on Windows it is not possible for the user
to create a symlink). It turns out we were overly-broad in which
exceptions lead to this fallback, and the subsequent copy would
also fail: at least one case where this occurred is when we
attempted to create a symlink that already existed.
The updated logic expressly avoids falling back to a copy when the
file/symlink already exists.
* ASP-based solver: use satisfies instead of intersects
They are semantically equivalent for concrete versions,
but the GitVersion.intersects implementation is buggy
* Mitigation for git version bug
fixes#36134
This commit works around the issue in #36134, by using
GitVersion.satisfies instead of GitVersion.intersects
There are still underlying issues when trying to infer the
"reference version" when no explicit one is given, but:
1. They are not reproducible with our synthetic repo
2. They occur only when the `git.<xxx>` form of Git version
is used
Here we just work around the user facing issue and ensure
the tests are correct with our synthetic repository.
This PR does 2 unrelated things:
1. It changes the encoding of the compilers
2. It tweaks the heuristic for the solves in a0d88179074f81d13a3fad629a43d86334e7f982
Both were initially motivated by trying to get a performance gain but, while 2 showed significant speed-ups[^1], 1 instead didn't. I kept it anyhow, since I think the code related to compilers is more consolidated with the new encoding and we might get some performance improvement out of it if we can base our errors on the `node_compiler(Package, CompilerID)` atoms instead of `attrs`.
[^1]: In general the changes in the heuristic brought a ~10% speed-up on the tests I did. I'll post detailed results below.
Add a warning about compilers.yaml that is triggered if there are multiple compilers with the same spec, os and
target (since they can't be selected by users with the spec syntax only).
Since GPG clear-sign cannot deal with lines longer than 19995 characters
and doesn't even error but simply truncates those linese (don't ask me
why...), we have to be careful not to hit that line limit when reducing
the filesize.
So, instead this PR sets the indent level to 0 and drops the whitespace
after `: `, which still reduces file size by 50% or so.
Adds builders appropriate for building these packages on Windows.
It is intended that builds on other platforms are unaffected (e.g.
they build with Autotools as before on Linux).
... and use colors in disambiguate message for clarity.
This commit avoids the loop:
```
for root in roots:
for dep in deps(root):
...
```
instead it ensures each node is visited once and only once.
Also adds a small optimization when searching for concrete specs, since
we can assume uniqueness of dag hash, so it's fine to early exit.
This adds a new mode for `concretizer:reuse` called `dependencies`,
which only reuses dependencies. Currently, `spack install foo` will
reuse older versions of `foo`, which might be surprising to users.
* CI configuration boilerplate reduction and refactor
Configuration:
- New notation for list concatenation (prepend/append)
- New notation for string concatenation (prepend/append)
- Break out configuration files for: ci.yaml, cdash.yaml, view.yaml
- Spack CI section refactored to improve self-consistency and
composability
- Scripts are now lists of lists and/or lists of strings
- Job attributes are now listed under precedence ordered list that are
composed/merged using Spack config merge rules.
- "service-jobs" are identified explicitly rather than as a batch
CI:
- Consolidate common, platform, and architecture configurations for all CI stacks into composable configuration files
- Make padding consistent across all stacks (256)
- Merge all package -> runner mappings to be consistent across all
stacks
Unit Test:
- Refactor CI module unit-tests for refactor configuration
Docs:
- Add docs for new notations in configuration.rst
- Rewrite docs on CI pipelines to be consistent with refactored CI
workflow
* Script verbose environ, dev bootstrap
* Port #35409
When untouched spec pruning is enabled, specs possibly affected
by a change cannot be pruned from a pipeline.
Previously spack looked at all specs matching changed package
names, and traversed dependents of each, all the way to the
environment root, to compute the set of environment specs
possibly affected by a change (and thus, not candidates for
pruning).
With this PR, when untouched spec pruning is enabled, a new
environment variable can control how far towards the root spack
traverses to compute the set of specs possibly affected by a
change. SPACK_UNTOUCHED_PRUNING_DEPENDENT_DEPTH can be set
to any numeric value before the "spack ci generate" command
is called to control this traversal depth parameter. Setting
it to "0" traverses only touched specs, setting it to "1"
traverses only touched specs and their direct dependents, and
so on. Omitting the variable results in the previous behavior
of traversing all the way to the root. Setting it to a negative
value means no traversal is done, and always yields an empty
set of possibly affected specs (which would result in the max
pruning possible).
Currently `spack buildcache create` creates compressed tarballs that
differ between each invocation, thanks to:
1. The gzip header containing mtime set to time.time()
2. The generated buildinfo file which has a different mtime every time.
To avoid this, you have to explicitly construct GZipFile yourself, since
the Python API doesn't expose the mtime arg, and we have to manually
create the tarinfo object for the buildinfo metadata file.
Normalize mode: regular files & hardlinks executable by user, dirs, symlinks: set 0o755 permissions in tarfile; other files use 0o644
This commit formalizes `satisfies(lhs, rhs, strict=True/False)`
and splits it into two functions: `satisfies(lhs, rhs)` and
`intersects(lhs, rhs)`.
- `satisfies(lhs, rhs)` means: all concrete specs matching the
left hand side also match the right hand side
- `intersects(lhs, rhs)` means: there exist concrete specs
matching both lhs and rhs.
`intersects` now has the property that it's commutative,
which previously was not guaranteed.
For abstract specs, `intersects(lhs, rhs)` implies that
`constrain(lhs, rhs)` works.
What's *not* done in this PR is ensuring that
`intersects(concrete, abstract)` returns false when the
abstract spec has additional properties not present in the
concrete spec, but `constrain(concrete, abstract)` will
raise an error.
To accomplish this, some semantics have changed, as well
as bugfixes to ArchSpec:
- GitVersion is now interpreted as a more constrained
version
- Compiler flags are interpreted as strings since their
order is important
- Abstract specs respect variant type (bool / multivalued)
Two fixes:
1. `-Wl,a,b,c,d` is a comma separated list of linker arguments, we
incorrectly assume key/value pairs, which runs into issues with for
example `-Wl,--enable-new-dtags,-rpath,/x`
2. `-Xlinker,xxx` is not a think, so it shouldn't be parsed.
Currently, if two compilers with the same spec differ on the flags, the concretizer will:
1. mix both sets of flags for the spec in the ASP program
2. error noting that the set of flags from the compiler (both of them) doesn't match the set from the lower priority compiler
This PR fixes both -- only flags from the highest priority compiler with a given spec are considered.
`mypy` only understands `sys.platform == "win32"`, not indirect assignments of that
value to things like `is_windows`. If we don't use the accepted platform checks, `mypy`
registers many Windows-only symbols as not present on Linux, when it should skip the
checks for platform-specific code.
Update `spack.util.environment` to remove legacy idioms.
* Remove kwargs from method signature and use a class for traces
* Uppercase a few global variables
* spack.util.environment: add type-hints
* Improve docstrings
* Fixed most style issues reported by pylint
a shared library /lib64/libcxi.so, which seems to also appear on other
non-slingshot systems. This patch also checks to make sure that there
is a Cray programming enviornment in /opt/cray/pe in addition to the
shared library.
if dump file existed it was not truncating the file, resulting in
a file with unaltered filesize, with the new content at the beginning,
"padded" with the tail of the old content, since the new content was
not enough to overwrite it.
`colify` is an old module in Spack that still uses `**kwargs` liberally.
We should be more explicit. Doing this eliminates the need for many
checks (can't pass the wrong arg if it isn't allowed) and makes the
function documentation more clear.
Fixes a bug introduced in 44ed0de8c0
where the push method of binary_distribution now takes named args
include_root and include_depedencies, to avoid the **kwarg hole.
But the call site wasn't update and we passed a dict of keys/values instead
of arguments, which resulted in a call like this:
```
push(include_root={"include_root": True, "include_dependencies": False})
```
This commit fixes that, and adds a test to see if we push the correct packages.
This error shows up a lot, typically it's harmless because an error
happened before the source build even started, in which case we don't
have build logs to copy. So, warn instead of error, cause it distracts
from the actual CI error.
Currently we attempt to setup the build environment even when
dependencies are not installed, which typically results in error while
searching for libraries or executables in a dependency's prefix.
With this change, we get a more user friendly error:
```
$ spack build-env perl
==> Error: Not all dependencies of perl are installed, cannot setup build environment:
- qpj6dw5 perl@5.36.0%apple-clang@14.0.0+cpanm+open+shared+threads build_system=generic arch=darwin-ventura-m1
- jq2plbe ^berkeley-db@18.1.40%apple-clang@14.0.0+cxx~docs+stl build_system=autotools patches=26090f4,b231fcc arch=darwin-ventura-m1
...
$ echo $?
1
```
* Allow users to specify root env dir
Environments managed by spack have some advantages over anonymous Environments
but they are tucked away inside spack's directory tree. This PR gives
users the ability to specify where the environments should live.
See #32823
This is also taken as an opportunity to ensure that all references are to "managed environments",
rather than "named environments". Prior to this PR some references to the latter persisted.
Co-authored-by: Tom Scogland <scogland1@llnl.gov>
Co-authored-by: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com>
Co-authored-by: Gregory Becker <becker33@llnl.gov>
The call:
```
x.satisfies(y[, strict=False])
```
is commutative, and tests non-empty intersection, whereas:
```
x.satsifies(y, strict=True)
```
is not commutative, and tests set-inclusion.
There are 2 fast paths. When strict=False both self and other need to
be concrete, when strict=True we can optimize when other is concrete.
Spack generally ignores file-file projection clashes in environment
views, but would eventually error when linking the `.spack` directory
for two specs of the same package.
This leads to obscure errors where users have no clue what the issue is
and how to fix it. On top of that, the error comes very late, since it
happens when the .spack dir contents are linked (which happens after
everything else)
This PR improves that by doing a quick check ahead of time if clashes
are going to be anticipated (by simply checking for clashes in the
projection of each spec's .spack metadir). If there are clashes, a
human-readable error is thrown which shows two of the conflicting specs,
and tells users to user unify:true, view:false, or set up custom
projections.
The checksum exception was not detailed enough and not reraised when using cache only, resulting in useless error messages.
Now it dumps the file path, expected
hash, computed hash, and the downloaded file summary.
* Style: black 23, skip magic trailing commas
* isort should use same line length as black
* Fix unused import
* Update version of black used in CI
* Update new packages
* Update new packages
Specs that did not contribute any files to an env view caused a problem
where zip(specs, files grouped by prefix) got "out of sync", causing the
wrong merge map to be passed to a package's `add_files_to_view`, which
specifically caused an issue where *sometimes* bin/python ended up as a
symlink instead of a copy.
One such example is kokkos + kokkos-nvcc-wrapper, as the latter package
only provides the file bin/nvcc_wrapper, which is also added to view by
kokkos, causing kokkos-nvcc-wrapper to contribute 0 files.
The test feels a bit contrived, but it captures the problem... pkg a is
added first and has 0 files to contribute, pkg b adds a single file, and
we check if pkg b receives a merge map (and a does not).
`spack gc` removes build deps of explicitly installed specs, but somehow
if you take one of the specs that `spack gc` would remove, and feed it
to `spack uninstall /<hash>` by hash, it complains about all the
dependents that still rely on it.
This resolves the inconsistency by only following run/link type deps in
spack uninstall.
That way you can finally do `spack uninstall cmake` without having to
remove all packages built with cmake.
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.
Account for group when enforcing requirements
packages:all : don't emit facts for requirement conditions
that can't apply to current spec
#35098 added the correct extraction of toolset version for the MSVC
compiler. This updates the associated method in MSBuilder to retrieve
the (now correct) property.
This PR enables the successful execution of the spack binary cache
tutorial on Windows. It assumes gnupg and file are available (they
can be installed with choco).
* Fix handling of args with quotes in spack.bat
* `file` utility can be installed on Windows (e.g. with choco): update
error message accordingly
At least with ZSH, prefix inspections containing `./bin` result in a
`$PREFIX/./bin` and result in strange `$PATH` handling.
I.e., `module load git` will prepend `/path/to/git/./bin`, `which git`
will find the right executable, but `git --version` will print the
system one. Normalize the relative path to avoid this behavior.
See also spack/spack#31867.
fixes#34879
This commit adds a new maintainer directive,
which by default extend the list of maintainers
for a given package.
The directive is backward compatible with the current
practice of having a "maintainers" list declared at
the class level.
Move the relocation of binary text in its own class
Drop threaded text replacement, since the current bottleneck
is decompression. It would be better to parallellize over packages,
instead of over files per package.
A small improvement with separate classes for text replacement is that we
now compile the regex in the constructor; previously it was compiled per
binary to be relocated.
This commit makes explicit the format version of the spec file
we are reading from.
Before there were different functions capable of reading some
part of the spec file at multiple format versions. The decision
was implicit, since checks were based on the structure of the
JSON without ever checking a format version number.
The refactor makes also explicit which spec file format is used
by which database and lockfile format, since the information is
stored in global mappings.
To ensure we don't change the hash of old specs, JSON representations
of specs have been added as data. A unit tests checks that we read
the correct hash in, and that the hash stays the same when we
re-serialize the spec using the most recent format version.
Co-authored-by: Harmen Stoppels <me@harmenstoppels.nl>
Currently we print "sha256 checksum failed for [file]. Expected X but
got Y".
This PR extends that message with file size and contents info:
"... but got Y. File size = 123456 bytes. Contents = b'abc...def'"
That way we can immediately see if the file was downloaded only
partially, or if we downloaded a text page instead of a binary, etc.
Co-authored-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
When running unit-test the test/ci.py module is leaving
garbage (help.sh, test.sh files) in the current working
directory.
This commit changes the current working directory to a
temporary path before those files are created.
* environments: don't rewrite relative view path, expand path on cli ahead of time
Currently if you have a spack.yaml that specifies a view by relative
path, Spack expands it to an absolute path on `spack -e . install` and
persists that to disk.
This is rather annoying when you have a `spack.yaml` file inside a git
repo, cause you want to use relative paths to make it relocatable, but
you constantly have to undo the changes made to spack.yaml by Spack.
So, as an alternative:
1. Always stick to paths as they are provided in spack.yaml, never
replace them with a canonicalized version
2. Turn relative paths on the command line into absolute paths before
storing to spack.yaml. This way you can do `spack env create --dir
./env --with-view ./view` and both `./env` and `./view` are resolved
to the current working dir, as expected (not `./env/view`). This
corresponds to the old behavior of `spack env create`.
* create --with-view always takes a value
All packages with explicit Windows support can be found with
`spack list --tags=windows`.
This also removes the documentation which explicitly lists
supported packages on Windows (which is currently out of date and
is now unnecessary with the added tags).
Note that if a package does not appear in this list, it *may*
still build on Windows, but it likely means that no explicit
attempt has been made to support it.
Since SPACK_PACKAGE_IDS is now also "namespaced" with <prefix>, it makes
more sense to call the flag `--make-prefix` and alias the old flag
`--make-target-prefix` to it.
Normally when using external packages in concretization, Spack ignores
all dependencies of the external. #33777 updated this logic to attach
a Python Spec to external Python extensions (most py-* packages), but
as implemented there were a couple issues:
* this did not account for concretization groups and could generate
multiple different python specs for a single DAG
* in some cases this created a fake Python spec with insufficient
details to be usable (concretization/installation of the
extension would fail)
This PR addresses both of these issues:
* For environment specs that are concretized together, external python
extensions in those specs will all be assigned the same Python spec
* If Spack needs to "invent" a Python spec, then it will have all the
needed details (e.g. compiler/architecture)
With the new variable [prefix/]SPACK_PACKAGE_IDS you can conveniently execute
things after each successful install.
For example push just-built packages to a buildcache
```
SPACK ?= spack
export SPACK_COLOR = always
MAKEFLAGS += -Orecurse
MY_BUILDCACHE := $(CURDIR)/cache
.PHONY: all clean
all: push
ifeq (,$(filter clean,$(MAKECMDGOALS)))
include env.mk
endif
# the relevant part: push has *all* example/push/<pkg identifier> as prereqs
push: $(addprefix example/push/,$(example/SPACK_PACKAGE_IDS))
$(SPACK) -e . buildcache update-index --directory $(MY_BUILDCACHE)
$(info Pushed everything, yay!)
# and each example/push/<pkg identifier> has the install target as prereq,
# and the body can use target local $(HASH) and $(SPEC) variables to do
# things, such as pushing to a build cache
example/push/%: example/install/%
@mkdir -p $(dir $@)
$(SPACK) -e . buildcache create --allow-root --only=package --unsigned --directory $(MY_BUILDCACHE) /$(HASH) # push $(SPEC)
@touch $@
spack.lock: spack.yaml
$(SPACK) -e . concretize -f
env.mk: spack.lock
$(SPACK) -e . env depfile -o $@ --make-target-prefix example
clean:
rm -rf spack.lock env.mk example/
``
In the past we checked remote binary mirrors for existence of a spec
before attempting to download it. That changed to only checking local
copies of index.jsons (if available) to prioritize certain mirrors where
we expect to find a tarball. That was faster for CI since fetching
index.json and loading it just to order mirrors takes more time than
just attempting to fetch tarballs -- and also if we have a direct hit
there's no point to look at other mirrors.
Long story short: the info message only makes sense in the old version
of Spack, so it's better to remove it.
* Forward lookup of "test_log_file" and "test_failures"
refers #34531closes#34487fixes#34440
* Add unit test
* py-libensemble: fix tests
* Support stand-alone tests with cached files as install-time tests
Co-authored-by: Tamara Dahlgren <dahlgren1@llnl.gov>
Ensure `spack mirror add <name> <url/path>` without further arguments translates to `<name>: <url>` key value pairs in mirrors.yaml. If --s3-* flags are provided, only store the provided ones.
Co-authored-by: Harmen Stoppels <me@harmenstoppels.nl>
With this change we get the invariant that `mirror.fetch_url` and
`mirror.push_url` return valid URLs, even when the backing config
file is actually using (relative) paths with potentially `$spack` and
`$env` like variables.
Secondly it avoids expanding mirror path / URLs too early,
so if I say `spack mirror add name ./path`, it stays `./path` in my
config. When it's retrieved through MirrorCollection() we
exand it to say `file://<env dir>/path` if `./path` was set in an
environment scope.
Thirdly, the interface is simplified for the relevant buildcache
commands, so it's more like `git push`:
```
spack buildcache create [mirror] [specs...]
```
`mirror` is either a mirror name, a path, or a URL.
Resolving the relevant mirror goes as follows:
- If it contains either / or \ it is used as an anonymous mirror with
path or url.
- Otherwise, it's interpreted as a named mirror, which must exist.
This helps to guard against typos, e.g. typing `my-mirror` when there
is no such named mirror now errors with:
```
$ spack -e . buildcache create my-mirror
==> Error: no mirror named "my-mirror". Did you mean ./my-mirror?
```
instead of creating a directory in the current working directory. I
think this is reasonable, as the alternative (requiring that a local dir
exists) feels a bit pendantic in the general case -- spack is happy to
create the build cache dir when needed, saving a `mkdir`.
The old (now deprecated) format will still be available in Spack 0.20,
but is scheduled to be removed in 0.21:
```
spack buildcache create (--directory | --mirror-url | --mirror-name) [specs...]
```
This PR also touches `tmp_scope` in tests, because it didn't really
work for me, since spack fixes the possible --scope values once and
for all across tests, so tests failed when run out of order.
Sometimes I just want to know how many packages of a certain type there are.
- [x] add `--count` option to `spack list` that output the number of packages that
*would* be listed.
```console
> spack list --count
6864
> spack list --count py-
2040
> spack list --count r-
1162
```
Currently, all of the replacements in `spack.util.path.replacements()` get evaluated for
each replacement. This makes it easy to get bootstrap issues, because config is used
very early on in Spack.
Right now, if I run `test_autotools_gnuconfig_replacement_no_gnuconfig` on my M1 mac, I
get the circular reference error below. This fixes the issue by making all of the path
replacements lazy lambdas.
As a bonus, this cleans up the way we do substitution for `$env` -- it's consistent with
other substitutions now.
- [x] make all path `replacements()` lazy
- [x] clean up handling of `$env`
```console
> spack unit-test -k test_autotools_gnuconfig_replacement_no_gnuconfig
...
==> [2022-12-31-15:44:21.771459] Error: AttributeError:
The 'autotools-config-replacement' package cannot find an attribute while trying to build from sources. This might be due to a change in Spack's package format to support multiple build-systems for a single package. You can fix this by updating the build recipe, and you can also report the issue as a bug. More information at https://spack.readthedocs.io/en/latest/packaging_guide.html#installation-procedure
/Users/gamblin2/src/spack/lib/spack/spack/package_base.py:1332, in prefix:
1330 @property
1331 def prefix(self):
>> 1332 """Get the prefix into which this package should be installed."""
1333 return self.spec.prefix
Traceback (most recent call last):
File "/Users/gamblin2/src/spack/lib/spack/spack/build_environment.py", line 1030, in _setup_pkg_and_run
kwargs["env_modifications"] = setup_package(
^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/build_environment.py", line 757, in setup_package
set_module_variables_for_package(pkg)
File "/Users/gamblin2/src/spack/lib/spack/spack/build_environment.py", line 596, in set_module_variables_for_package
m.std_cmake_args = spack.build_systems.cmake.CMakeBuilder.std_args(pkg)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/build_systems/cmake.py", line 241, in std_args
define("CMAKE_INSTALL_PREFIX", pkg.prefix),
^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/package_base.py", line 1333, in prefix
return self.spec.prefix
^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/spec.py", line 1710, in prefix
self.prefix = spack.store.layout.path_for_spec(self)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/directory_layout.py", line 336, in path_for_spec
path = self.relative_path_for_spec(spec)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/directory_layout.py", line 106, in relative_path_for_spec
projection = spack.projections.get_projection(self.projections, spec)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/projections.py", line 13, in get_projection
if spec.satisfies(spec_like, strict=True):
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/spec.py", line 3642, in satisfies
if not self.virtual and other.virtual:
^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/spec.py", line 1622, in virtual
return spack.repo.path.is_virtual(self.name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/repo.py", line 890, in is_virtual
return have_name and pkg_name in self.provider_index
^^^^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/repo.py", line 770, in provider_index
self._provider_index.merge(repo.provider_index)
^^^^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/repo.py", line 1096, in provider_index
return self.index["providers"]
~~~~~~~~~~^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/repo.py", line 592, in __getitem__
self._build_all_indexes()
File "/Users/gamblin2/src/spack/lib/spack/spack/repo.py", line 607, in _build_all_indexes
self.indexes[name] = self._build_index(name, indexer)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/spack/repo.py", line 616, in _build_index
index_mtime = self.cache.mtime(cache_filename)
^^^^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/llnl/util/lang.py", line 826, in __getattr__
return getattr(self.instance, name)
^^^^^^^^^^^^^
File "/Users/gamblin2/src/spack/lib/spack/llnl/util/lang.py", line 825, in __getattr__
raise AttributeError()
AttributeError
```
Since we dropped support for Python 2.7, we can embrace using keyword only arguments
for many functions in Spack that use **kwargs in the function signature. Here this is done
for the llnl.util.filesystem module.
There were a couple of bugs lurking in the code related to typo-like errors when retrieving
from kwargs. Those have been fixed as well.
The code in FileCache for write_transaction attempts to delete the temporary file when an exception occurs under the context by calling shutil.rmtree. However, rmtree only operates on directories while the rest of FileCache uses normal files. This causes an empty file to be written to the cache key when unneeded.
Use os.remove instead which operates on normal files.
Co-authored-by: Harmen Stoppels <harmenstoppels@gmail.com>
Windows CMD prompt does not automatically support ASCI color control
characters on the console from Python. Enable this behavior by
accessing the current console and allowing the interpreation of ASCI
control characters from Python via the win32 API.
This commit allows (remote) spec.json files to be clearsigned and gzipped.
The idea is to reduce the number of requests and number of bytes transferred
The code in Spack to generate install and test reports currently suffers from unneeded complexity. For
instance, we have classes in Spack core packages, like `spack.reporters.CDash`, that need an
`argparse.Namespace` to be initialized and have "hard-coded" string literals on which they branch to
change their behavior:
```python
if do_fn.__name__ == "do_test" and skip_externals:
package["result"] = "skipped"
else:
package["result"] = "success"
package["stdout"] = fetch_log(pkg, do_fn, self.dir)
package["installed_from_binary_cache"] = pkg.installed_from_binary_cache
if do_fn.__name__ == "_install_task" and installed_already:
return
```
This PR attempt to polish the major issues encountered in both `spack.report` and `spack.reporters`.
Details:
- [x] `spack.reporters` is now a package that contains both the base class `Reporter` and all
the derived classes (`JUnit` and `CDash`)
- [x] Classes derived from `spack.reporters.Reporter` don't take an `argparse.Namespace` anymore
as argument to `__init__`. The rationale is that code for commands should be built upon Spack
core classes, not vice-versa.
- [x] An `argparse.Action` has been coded to create the correct `Reporter` object based on command
line arguments
- [x] The context managers to generate reports from either `spack install` or from `spack test` have
been greatly simplified, and have been made less "dynamic" in nature. In particular, the `collect_info`
class has been deleted in favor of two more specific context managers. This allows for a simpler
structure of the code, and less knowledge required to client code (in particular on which method to patch)
- [x] The `InfoCollector` class has been turned into a simple hierarchy, so to avoid conditional statements
within methods that assume a knowledge of the context in which the method is called.
On systems with remote groups, the primary user group may be remote and may not exist on
the local system (i.e., it might just be a number). On the CLI, it looks like this:
```console
> touch foo
> l foo
-rw-r--r-- 1 gamblin2 57095 0 Dec 29 22:24 foo
> chmod 2000 foo
chmod: changing permissions of 'foo': Operation not permitted
```
Here, the local machine doesn't know about per-user groups, so they appear as gids in
`ls` output. `57095` is also `gamblin2`'s uid, but the local machine doesn't know that
`gamblin2` is in the `57095` group.
Unfortunately, it seems that Python's `os.chmod()` just fails silently, setting
permissions to `0o0000` instead of `0o2000`. We can avoid this by ensuring that the file
has a group the user is known to be a member of.
- [x] Add `ensure_known_group()` in the permissions tests.
- [x] Call `ensure_known_group()` on tempfile in `test_chmod_real_entries_ignores_suid_sgid`.
There are a number of places in our docstrings where we write "list of X" as the type, even though napoleon doesn't actually support this. It ends up causing warnings when generating docs.
Now that we require Python 3, we don't have to rely on type hints in docs -- we can just use Python type hints and omit the types of args and return values from docstrings.
We should probably do this for all types in docstrings eventually, but this PR focuses on the ones that generate warnings during doc builds.
Some `mypy` annoyances we should consider in the future:
1. Adding some of these type annotations gets you:
```
note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs [annotation-unchecked]
```
because they are in unannotated functions (like constructors where we don't really need any annotations).
You can silence these with `disable_error_code = "annotation-unchecked"` in `pyproject.toml`
2. Right now we support running `mypy` in Python `3.6`. That means we have to support `mypy` `.971`, which does not support `disable_error_code = "annotation-unchecked"`, so I just filter `[annotation-unchecked]` lines out in `spack style`.
3. I would rather just turn on `check_untyped_defs` and get more `mypy` coverage everywhere, but that will require about 1,000 fixes. We should probably do that eventually.
4. We could also consider only running `mypy` on newer python versions. This is not easy to do while supporting `3.6`, because you have to use `if TYPE_CHECKING` for a lot of things to ensure that 3.6 still parses correctly. If we only supported `3.7` and above we could use [`from __future__ import annotations`](https://mypy.readthedocs.io/en/stable/runtime_troubles.html#future-annotations-import-pep-563), but we have to support 3.6 for now. Sigh.
- [x] Convert a number of docstring types to Python type hints
- [x] Get rid of "list of" wherever it appears
Local `git` tests will fail with `fatal: transport 'file' not allowed` when using git 2.38.1 or higher, due to a fix for `CVE-2022-39253`.
This was fixed in CI in #33429, but that doesn't help the issue for anyone's local environment. Instead of fixing this with git config in CI, we should ensure that the tests run anywhere.
- [x] Introduce `spack.util.git`.
- [x] Use `spack.util.git.get_git()` to get a git executable, instead of `which("git")` everywhere.
- [x] Make all `git` tests use a `git` fixture that goes through `spack.util.git.get_git()`.
- [x] Add `-c protocol.file.allow=always` to all `git` invocations under `pytest`.
- [x] Revert changes from #33429, which are no longer needed.
`spack graph` has been reworked to use:
- Jinja templates
- builder objects to construct the template context when DOT graphs are requested.
This allowed to add a new colored output for DOT graphs that highlights both
the dependency types and the nodes that are needed at runtime for a given spec.
`spack solve` is supposed to show you times you can compare. setup, ground, solve, etc.
all in a list. You're also supposed to be able to compare easily across runs. With
`pretty_seconds()` (introduced in #33900), it's easy to miss the units, e.g., spot the
bottleneck here:
```console
> spack solve --timers tcl
setup 22.125ms
load 16.083ms
ground 8.298ms
solve 848.055us
total 58.615ms
```
It's easier to see what matters if these are all in the same units, e.g.:
```
> spack solve --timers tcl
setup 0.0147s
load 0.0130s
ground 0.0078s
solve 0.0008s
total 0.0463s
```
And the units won't fluctuate from run to run as you make changes.
-[x] make `spack solve` timings consistent like before
Avoid text decoding and encoding when combining log files, instead
combine in binary mode.
Also do a buffered copy which is sometimes faster for large log files.
Currently, the Spack docs show documentation for submodules *before* documentation for
submodules on package doc pages. This means that if you put docs in `__init__.py` in
some package, the docs in there will be shown *after* the docs for all submodules of the
package instead of at the top as an intro to the package. See, e.g.,
[the lockfile docs](https://spack.readthedocs.io/en/latest/spack.environment.html#module-spack.environment),
which should be at the
[top of that page](https://spack.readthedocs.io/en/latest/spack.environment.html).
- [x] add the `--module-first` option to sphinx so that it generates module docs at top of page.
The sticky property will prevent clingo from changing the amdgpu_target
to work around conflicts. This is the same behaviour as was adopted for
cuda_arch in 055c9d125d.
Implement an alternative strategy to do index.json invalidation.
The current approach of pairs of index.json / index.json.hash is
problematic because it leads to races.
The standard solution for cache invalidation is etags, which are
supported by both http and s3 protocols, which allows one to do
conditional fetches.
This PR implements that for the http/https schemes. It should also work
for s3 schemes, but that requires other prs to be merged.
Also it improves unit tests for index.json fetches.
Interim fix for #34559
Spack's MSVC compiler definition uses ifx as the Fortran compiler.
Prior to #33385, the Spack MSVC compiler definition required the
executable to be called "ifx.exe"; #33385 replaced this with just
"ifx", which inadvertently led to ifx falsely indicating the
presence of MSVC on non-Windows systems (which leads to future
errors when attempting to query/use those compiler objects).
This commit applies a short-term fix by updating MSVC Fortran
version detection to always indicate a failure on non-Windows.
fixes#34518
Fix an issue due to the MRO chain of the package wrapper
during build. Before this PR we were always returning
False when the builder object was created before the
run_tests method was monkey patched.
This reverts commit 8035eeb36d.
And also removes logic around an additional HEAD request to prevent
a more expensive GET request on wrong content-type. Since large files
are typically an attachment and only downloaded when reading the
stream, it's not an optimization that helps much, and in fact the logic
was broken since the GET request was done unconditionally.
The main issue that's fixed is that Spack passes paths (as strings) to
functions that require urls. That wasn't an issue on unix, since there
you can simply concatenate `file://` and `path` and all is good, but on
Windows that gives invalid file urls. Also on Unix, Spack would not deal with uri encoding like x%20y for file paths.
It also removes Spack's custom url.parse function, which had its own incorrect interpretation of file urls, taking file://x/y to mean the relative path x/y instead of hostname=x and path=/y. Also it automatically interpolated variables, which is surprising for a function that parses URLs.
Instead of all sorts of ad-hoc `if windows: fix_broken_file_url` this PR
adds two helper functions around Python's own path2url and reverse.
Also fixes a bug where some `spack buildcache` commands
used `-d` as a flag to mean `--mirror-url` requiring a URL, and others
`--directory`, requiring a path. It is now the latter consistently.
When installing binary tarballs, Spack has to download from its
binary mirrors.
Sometimes Spack has cache available for these mirrors.
That cache helps to order mirrors to increase the likelihood of
getting a direct hit.
However, currently, when Spack can't find a spec in any local cache
of mirrors, it's very dumb:
- A while ago it used to query each mirror to see if it had a spec,
and use that information to order the mirror again, only to go
about and do exactly a part of what it just did: fetch the spec
from that mirror confused
- Recently, it was changed to download a full index.json, which
can be multiple dozens of MBs of data and may take a minute to
process thanks to the blazing fast performance you get with
Python.
In a typical use case of concretizing with reuse, the full index.json
is already available, and it likely that the local cache gives a perfect
mirror ordering on install. (There's typically no need to update any
caches).
However, in the use case of Gitlab CI, the build jobs don't have cache,
and it would be smart to just do direct fetches instead of all the
redundant work of (1) and/or (2).
Also, direct fetches from mirrors will soon be fast enough to
prefer these direct fetches over the excruciating slowness of
index.json files.
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
```
* `url_exists` improvements (take 2)
Make `url_exists` do HEAD request for http/https/s3 protocols
Rework the opener: construct it once and only once, dynamically dispatch
to the right one based on config.
It's very common for us to tell users to grep through the existing Spack packages to
find examples of what they want, and it's also very common for package developers to do
it. Now, searching packages is even easier.
`spack pkg grep` runs grep on all `package.py` files in repos known to Spack. It has no
special options other than the search string; all options passed to it are forwarded
along to `grep`.
```console
> spack pkg grep --help
usage: spack pkg grep [--help] ...
positional arguments:
grep_args arguments for grep
options:
--help show this help message and exit
```
```console
> spack pkg grep CMakePackage | head -3
/Users/gamblin2/src/spack/var/spack/repos/builtin/packages/3dtk/package.py:class _3dtk(CMakePackage):
/Users/gamblin2/src/spack/var/spack/repos/builtin/packages/abseil-cpp/package.py:class AbseilCpp(CMakePackage):
/Users/gamblin2/src/spack/var/spack/repos/builtin/packages/accfft/package.py:class Accfft(CMakePackage, CudaPackage):
```
```console
> spack pkg grep -Eho '(\S*)\(PythonPackage\)' | head -3
AwsParallelcluster(PythonPackage)
Awscli(PythonPackage)
Bueno(PythonPackage)
```
## Return Value
This retains the return value semantics of `grep`:
* 0 for found,
* 1 for not found
* >1 for error
## Choosing a `grep`
You can set the ``SPACK_GREP`` environment variable to choose the ``grep``
executable this command should use.
Unit tests on Windows are supposed to pass for any PR to pass CI.
However, the return code for the unit test command was not being
checked, which meant this check was always passing (effectively
disabled). This PR
* Properly checks the result of the unit tests and fails if the
unit tests fail
* Fixes (or disables on Windows) a number of tests which have
"drifted" out of support on Windows since this check was
effectively disabled
## Motivation
Our parser grew to be quite complex, with a 2-state lexer and logic in the parser
that has up to 5 levels of nested conditionals. In the future, to turn compilers into
proper dependencies, we'll have to increase the complexity further as we foresee
the need to add:
1. Edge attributes
2. Spec nesting
to the spec syntax (see https://github.com/spack/seps/pull/5 for an initial discussion of
those changes). The main attempt here is thus to _simplify the existing code_ before
we start extending it later. We try to do that by adopting a different token granularity,
and by using more complex regexes for tokenization. This allow us to a have a "flatter"
encoding for the parser. i.e., it has fewer nested conditionals and a near-trivial lexer.
There are places, namely in `VERSION`, where we have to use negative lookahead judiciously
to avoid ambiguity. Specifically, this parse is ambiguous without `(?!\s*=)` in `VERSION_RANGE`
and an extra final `\b` in `VERSION`:
```
@ 1.2.3 : develop # This is a version range 1.2.3:develop
@ 1.2.3 : develop=foo # This is a version range 1.2.3: followed by a key-value pair
```
## Differences with the previous parser
~There are currently 2 known differences with the previous parser, which have been added on purpose:~
- ~No spaces allowed after a sigil (e.g. `foo @ 1.2.3` is invalid while `foo @1.2.3` is valid)~
- ~`/<hash> @1.2.3` can be parsed as a concrete spec followed by an anonymous spec (before was invalid)~
~We can recover the previous behavior on both ones but, especially for the second one, it seems the current behavior in the PR is more consistent.~
The parser is currently 100% backward compatible.
## Error handling
Being based on more complex regexes, we can possibly improve error
handling by adding regexes for common issues and hint users on that.
I'll leave that for a following PR, but there's a stub for this approach in the PR.
## Performance
To be sure we don't add any performance penalty with this new encoding, I measured:
```console
$ spack python -m timeit -s "import spack.spec" -c "spack.spec.Spec(<spec>)"
```
for different specs on my machine:
* **Spack:** 0.20.0.dev0 (c9db4e50ba045f5697816187accaf2451cb1aae7)
* **Python:** 3.8.10
* **Platform:** linux-ubuntu20.04-icelake
* **Concretizer:** clingo
results are:
| Spec | develop | this PR |
| ------------- | ------------- | ------- |
| `trilinos` | 28.9 usec | 13.1 usec |
| `trilinos @1.2.10:1.4.20,2.0.1` | 131 usec | 120 usec |
| `trilinos %gcc` | 44.9 usec | 20.9 usec |
| `trilinos +foo` | 44.1 usec | 21.3 usec |
| `trilinos foo=bar` | 59.5 usec | 25.6 usec |
| `trilinos foo=bar ^ mpich foo=baz` | 120 usec | 82.1 usec |
so this new parser seems to be consistently faster than the previous one.
## Modifications
In this PR we just substituted the Spec parser, which means:
- [x] Deleted in `spec.py` the `SpecParser` and `SpecLexer` classes. deleted `spack/parse.py`
- [x] Added a new parser in `spack/parser.py`
- [x] Hooked the new parser in all the places the previous one was used
- [x] Adapted unit tests in `test/spec_syntax.py`
## Possible future improvements
Random thoughts while working on the PR:
- Currently we transform hashes and files into specs during parsing. I think
we might want to introduce an additional step and parse special objects like
a `FileSpec` etc. in-between parsing and concretization.
This commit reworks the bootstrapping procedure to use Spack environments
as much as possible.
The `spack.bootstrap` module has also been reorganized into a Python package.
A distinction is made among "core" Spack dependencies (clingo, GnuPG, patchelf)
and other dependencies. For a number of reasons, explained in the `spack.bootstrap.core`
module docstring, "core" dependencies are bootstrapped with the current ad-hoc
method.
All the other dependencies are instead bootstrapped using a Spack environment
that lives in a directory specific to the interpreter and the architecture being used.
All the vermin annotations we were using were for optional features introduced in early
Python 3 versions. We no longer need any of them, as we only support Python 3.6+. If we
start optionally using features from newer Pythons than 3.6, we will need more vermin
annotations.
Co-authored-by: Harmen Stoppels <harmenstoppels@gmail.com>
Co-authored-by: Harmen Stoppels <harmenstoppels@gmail.com>
We no longer support Python <3.6, so we don't need to check whether Python supports SSL
verification in `spack.util.web`.
- [x] Remove a bunch of logic we needed to appease Python 2
We've stopped supporting Python 2, and contributors are noticing that our CI no longer
allows Python 2.7 comment type hints. They end up having to adapt them, but this adds
extra unrelated work to PRs.
- [x] Move to 3.6 type hints across the entire code base
All Spec attributes are now represented as `attr(attribute_name, ... args ...)`, e.g.
`attr(node, "hdf5")` instead of `node("hdf5")`, as we *have* to maintain the `attr()`
form anyway, and it simplifies the encoding to just maintain one form of the Spec
information.
Background
----------
In #20644, we unified the way conditionals are done in the concretizer, but this
introduced a nasty aspect to the encoding: we have to maintain everything we want in
general conditions in two forms: `predicate(...)` and `attr("predicate", ...)`. For
example, here's the start of the table of spec attributes we had to maintain:
```prolog
node(Package) :- attr("node", Package).
virtual_node(Virtual) :- attr("virtual_node", Virtual).
hash(Package, Hash) :- attr("hash", Package, Hash).
version(Package, Version) :- attr("version", Package, Version).
...
```
```prolog
attr("node", Package) :- node(Package).
attr("virtual_node", Virtual) :- virtual_node(Virtual).
attr("hash", Package, Hash) :- hash(Package, Hash).
attr("version", Package, Version) :- version(Package, Version).
...
```
This adds cognitive load to understanding how the concretizer works, as you have to
understand the equivalence between the two forms of spec attributes. It also makes the
general condition logic in #20644 hard to explain, and it's easy to forget to add a new
equivalence to this list when adding new spec attributes (at least two people have been
bitten by this).
Solution
--------
- [x] remove the equivalence list from `concretize.lp`
- [x] simplify `spec_clauses()`, `condition()`, and other functions in `asp.py` that need
to deal with `Spec` attributes.
- [x] Convert all old-form spec attributes in `concretize.lp` to the `attr()` form
- [x] Simplify `display.lp`, where we also had to maintain a list of spec attributes. Now
we only need to show `attr/2`, `attr/3`, and `attr/4`.
- [x] Simplify model extraction logic in `asp.py`.
Performance
-----------
This seems to result in a smaller grounded problem (as there are no longer duplicated
`attr("foo", ...)` / `foo(...)` predicates in the program), but it also adds a slight
performance overhead vs. develop. Ultimately, simplifying the encoding will be a win,
particularly for improving error messages.
Notes
-----
This will simplify future node refactors in `concretize.lp` (e.g., not identifying nodes
by package name, which we need for separate build dependencies).
I'm still not entirely used to reading `attr()` notation, but I thnk it's ultimately
clearer than what we did before. We need more uniform naming, and it's now clear what is
part of a solution. We should probably continue making the encoding of `concretize.lp`
simpler and more self-explanatory. It may make sense to rename `attr` to something like
`node_attr` and to simplify the names of node attributes. It also might make sense to do
something similar for other types of predicates in `concretize.lp`.
This reverts commit d06fd26c9a.
The problem is that Bitbucket's API forwards download requests to an S3 bucket using a temporary URL. This URL includes a signature for the request, which embeds the HTTP verb. That means only GET requests are allowed, and HEAD requests would fail verification, leading to 403 erros. The same is observed when using `curl -LI ...`
Using `-Werror` is good practice for development and testing, but causes us a great
deal of heartburn supporting multiple compiler versions, especially as newer compiler
versions add warnings for released packages. This PR adds support for suppressing
`-Werror` through spack's compiler wrappers. There are currently three modes for
the `flags:keep_werror` setting:
* `none`: (default) cancel all `-Werror`, `-Werror=*` and `-Werror-*` flags by
converting them to `-Wno-error[=]*` flags
* `specific`: preserve explicitly selected warnings as errors, such as
`-Werror=format-truncation`, but reverse the blanket `-Werror`
* `all`: keeps all `-Werror` flags
These can be set globally in config.yaml, through the config command-line flags, or
overridden by a particular package (some packages use Werror as a proxy for determining
support for other compiler features). We chose to use this approach because:
1. removing `-Werror` flags entirely broke *many* build systems, especially autoconf
based ones, because of things like checking `-Werror=feature` and making the
assumption that if that did not error other flags related to that feature would also work
2. Attempting to preserve `-Werror` in some phases but not others caused similar issues
3. The per-package setting came about because some packages, even with all these
protections, still use `-Werror` unsafely. Currently there are roughly 3 such packages
known.
For reasons beyond me Python thinks it's a great idea to upgrade HEAD
requests to GET requests when following redirects. So, this PR adds a
better `HTTPRedirectHandler`, and also moves some ad-hoc logic around
for dealing with disabling SSL certs verification.
Also, I'm stumped by the fact that Spack's `url_exists` does not use
HEAD requests at all, so in certain cases Spack awkwardly downloads
something first to see if it can download it, and then downloads it
again because it knows it can download it. So, this PR ensures that both
urllib and botocore use HEAD requests.
Finally, it also removes some things that were there to support currently
unsupported Python versions.
Notice that the HTTP spec [section 10.3.2](https://datatracker.ietf.org/doc/html/rfc2616.html#section-10.3.2) just talks about how to deal
with POST request on redirect (whether to follow or not):
> If the 301 status code is received in response to a request other
> than GET or HEAD, the user agent MUST NOT automatically redirect the
> request unless it can be confirmed by the user, since this might
> change the conditions under which the request was issued.
> Note: When automatically redirecting a POST request after
> receiving a 301 status code, some existing HTTP/1.0 user agents
> will erroneously change it into a GET request.
Python has a comment about this, they choose to go with the "erroneous change".
But they then mess up the HEAD request while following the redirect, probably
because they were too busy discussing how to deal with POST.
See https://github.com/python/cpython/pull/99731
This adds super-lazy maintainer mode to `spack checksum`: Instead of
only printing the new checksums to the terminal, `-a` and
`--add-to-package` will add the new checksums to the `package.py` file
and open it in the editor afterwards for final checks.
