Fix corrupted package cache for outputs in subpackage tests #5184
Conversation
conda-bot
added
the
cla-signed
mbargull
force-pushed
the
fix-multi-output-package-corruption
branch
from
1fcbd9a to
dc95fee
Compare
mbargull
changed the title
mbargull
force-pushed
the
fix-multi-output-package-corruption
branch
from
a7ab905 to
dd90768
Compare
|
cc @h-vetinari, What happened in #5179 (comment) / conda-forge/boost-feedstock#189 (comment) was that one of the subpackage's build changed files from the base subpackage it depends on ( This didn't produce corrupted package files, but rather only corrupted the extracted packages in the package cache.
Options 1 & 2 are disk space efficient, of course, but allow potential corruption of shared files. To have a space efficient but also corruption resistant option, |
| # Remove any previously cached build from the package cache to ensure we | ||
| # really test the requested build and not some clashing or corrupted build. | ||
| # (Corruption of the extracted package can happen, e.g., in multi-output | ||
| # builds if one of the subpackages overwrites files from the other.) | ||
| # Special case: | ||
| # If test is requested for .tar.bz2/.conda file from the pkgs dir itself, | ||
| # clean_pkg_cache() will remove it; don't call that function in this case. | ||
| in_pkg_cache = ( | ||
| not hasattr(recipedir_or_package_or_metadata, "config") | ||
| and os.path.isfile(recipedir_or_package_or_metadata) | ||
| and recipedir_or_package_or_metadata.endswith(CONDA_PACKAGE_EXTENSIONS) | ||
| and any( | ||
| os.path.dirname(recipedir_or_package_or_metadata) in pkgs_dir | ||
| for pkgs_dir in pkgs_dirs | ||
| ) | ||
| ) | ||
| if not in_pkg_cache: | ||
| environ.clean_pkg_cache(metadata.dist(), metadata.config) | ||
|
|
There was a problem hiding this comment.
This code is the one removed in gh-5031 , but with a more descriptive comment and changed to handle all entries from pkgs_dirs (because environ.clean_pkg_cache does now).
conda_build/environ.py
Outdated
| def clean_pkg_cache(dist, config): | ||
| with utils.LoggingContext(logging.DEBUG if config.debug else logging.WARN): | ||
| locks = get_pkg_dirs_locks([config.bldpkgs_dir] + pkgs_dirs, config) | ||
| with utils.try_acquire_locks(locks, timeout=config.timeout): | ||
| for pkgs_dir in pkgs_dirs: | ||
| if any( | ||
| os.path.exists(os.path.join(pkgs_dir, f"{dist}{ext}")) | ||
| for ext in ("", *CONDA_PACKAGE_EXTENSIONS) | ||
| ): | ||
| log.debug( | ||
| "Conda caching error: %s package remains in cache after removal", | ||
| dist, | ||
| ) | ||
| log.debug("manually removing to compensate") | ||
| package_cache = PackageCacheData.first_writable([pkgs_dir]) | ||
| for cache_pkg_id in package_cache.query(dist): | ||
| package_cache.remove(cache_pkg_id) | ||
|
|
||
| # Note that this call acquires the relevant locks, so this must be called | ||
| # outside the lock context above. | ||
| remove_existing_packages(pkgs_dirs, [dist], config) | ||
|
|
||
|
|
There was a problem hiding this comment.
This is essentially what https://github.com/conda/conda-build/pull/5151/files#diff-3ba52da5dde963be464da17aa3a40b91541d9709735c97fd03b59d3a0fb54a78L1223-L1266 did, just removed unnecessary Dist stuff and handles multiple pkgs_dirs and .conda better.
Signed-off-by: Marcel Bargull <[email protected]>
This re-introduces conda_build.environ.clean_pkg_cache with slight changes to not use conda.models.dist.Dist and handle multiple pkgs_dirs better. Signed-off-by: Marcel Bargull <[email protected]>
mbargull
force-pushed
the
fix-multi-output-package-corruption
branch
from
dd90768 to
34552d5
Compare
|
@kenodegard, I've added this to #5134 (comment) for |
| ) | ||
| ) | ||
| if not in_pkg_cache: | ||
| environ.clean_pkg_cache(metadata.dist(), metadata.config) |
There was a problem hiding this comment.
N.B.: This only ensures the current to-be-tested package gets cleaned.
I.e., if some other package's files are altered in the package cache, this is not handled (we didn't handle this in <=3.27 either).
E.g., in the added test case below we only alter the package file during build; if we were do it during the test (package first-... overwriting file from base-...), then the subsequent test of second-... would fail since only second-... but not base-... is passed on to clean_pkg_cache.
| dist, | ||
| ) | ||
| log.debug("manually removing to compensate") | ||
| package_cache = PackageCacheData.first_writable([pkgs_dir]) |
There was a problem hiding this comment.
could the cached package exist in a one of the non-writable locations?
There was a problem hiding this comment.
Yes, and it would raise a NoWritablePkgsDirError then.
We have 2 options here:
- Let is fail somehow (we may want to have better error handling at somepoint),
- Ignore the existing package and risk to test a different package build we didn't intend to test.
Signed-off-by: Marcel Bargull <[email protected]> Co-authored-by: Ken Odegard <[email protected]>
ref: - conda/conda-build#5184 Signed-off-by: Marcel Bargull <[email protected]>
ref: - conda/conda-build#5184 Signed-off-by: Marcel Bargull <[email protected]>
ref: - conda/conda-build#5184 Signed-off-by: Marcel Bargull <[email protected]>
Description
Fix corrupted package cache for outputs in subpackage tests
This re-introduces conda_build.environ.clean_pkg_cache with slight
changes to not use conda.models.dist.Dist and handle multiple pkgs_dirs
better.
Addresses #5179 (comment)
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?[ ] Add / update outdated documentation?