Skip to content
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

Fix corrupted package cache for outputs in subpackage tests #5184

Merged
merged 3 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions conda_build/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
env_path_backup_var_exists,
get_conda_channel,
get_rc_urls,
pkgs_dirs,
prefix_placeholder,
reset_context,
root_dir,
Expand Down Expand Up @@ -3394,6 +3395,25 @@ def test(
# folder destination
_extract_test_files_from_package(metadata)

# 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")
kenodegard marked this conversation as resolved.
Show resolved Hide resolved
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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Comment on lines +3398 to +3416
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

copy_test_source_files(metadata, metadata.config.test_dir)
# this is also copying tests/source_files from work_dir to testing workdir

Expand Down
31 changes: 30 additions & 1 deletion conda_build/environ.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,15 @@
from logging import getLogger
from os.path import join, normpath

from conda.base.constants import DEFAULTS_CHANNEL_NAME, UNKNOWN_CHANNEL
from conda.base.constants import (
CONDA_PACKAGE_EXTENSIONS,
DEFAULTS_CHANNEL_NAME,
UNKNOWN_CHANNEL,
)
from conda.common.io import env_vars
from conda.core.index import LAST_CHANNEL_URLS
from conda.core.link import PrefixSetup, UnlinkLinkTransaction
from conda.core.package_cache_data import PackageCacheData
from conda.core.prefix_data import PrefixData
from conda.models.channel import prioritize_channels

Expand All @@ -43,6 +48,7 @@
reset_context,
root_dir,
)
from .config import Config
from .deprecations import deprecated
from .exceptions import BuildLockError, DependencyNeedsBuildingError
from .features import feature_list
Expand Down Expand Up @@ -1264,6 +1270,29 @@ def get_pkg_dirs_locks(dirs, config):
return [utils.get_lock(folder, timeout=config.timeout) for folder in dirs]


def clean_pkg_cache(dist: str, config: Config) -> None:
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])
Copy link
Contributor

@kenodegard kenodegard Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could the cached package exist in a one of the non-writable locations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and it would raise a NoWritablePkgsDirError then.
We have 2 options here:

  1. Let is fail somehow (we may want to have better error handling at somepoint),
  2. Ignore the existing package and risk to test a different package build we didn't intend to test.

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)


def remove_existing_packages(dirs, fns, config):
locks = get_pkg_dirs_locks(dirs, config) if config.locking else []

Expand Down
19 changes: 19 additions & 0 deletions news/5184-fix-multi-output-package-corruption
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
### Enhancements

* <news item>

### Bug fixes

* Fix corrupted package cache for outputs in subpackage tests. (#5184)

### Deprecations

* <news item>

### Docs

* <news item>

### Other

* <news item>
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
:: Always output 4 characters to properly test even if "SafetyError: ... incorrect size." is not triggered.
< nul set /p="%PKG_NAME:~0,4%" > "%PREFIX%\file" & call;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
## Always output 4 characters to properly test even if "SafetyError: ... incorrect size." is not triggered.
printf '%.4s' "${PKG_NAME}" > "${PREFIX}/file"
40 changes: 40 additions & 0 deletions tests/test-recipes/metadata/outputs_overwrite_base_file/meta.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{% set name = "outputs_overwrite_base_file" %}

package:
name: {{ name }}
version: 1.0

outputs:
- name: base-{{ name }}
script: install.sh # [unix]
script: install.bat # [win]

- name: first-{{ name }}
script: install.sh # [unix]
script: install.bat # [win]
requirements:
host:
- {{ pin_subpackage("base-" + name) }}
run:
- {{ pin_subpackage("base-" + name) }}
test:
commands:
- content="$(cat "${PREFIX}/file")" # [unix]
- test "${content}" = base # [unix]
- < "%PREFIX%\file%" set /p content= # [win]
- if not "%content%" == "base" exit 1 # [win]

- name: second-{{ name }}
script: install.sh # [unix]
script: install.bat # [win]
requirements:
host:
- {{ pin_subpackage("base-" + name) }}
run:
- {{ pin_subpackage("base-" + name) }}
test:
commands:
- content="$(cat "${PREFIX}/file")" # [unix]
- test "${content}" = "base" # [unix]
- < "%PREFIX%\file%" set /p content= # [win]
- if not "%content%" == "base" exit 1 # [win]
Loading