Skip to content

Commit

Permalink
fix(builder)!: remove single-step non-strict install (#1727)
Browse files Browse the repository at this point in the history 
This separates out the install step for non strict-dependencies builds
as follows:

1. Install from `python-binary-packages`
2. Install (source-only) from `python-packages`
3. Install (source-only) from requirements files and charm libs.
  • Loading branch information
@lengau
lengau authored Jul 19, 2024
1 parent 1f6730c commit 51a55f7
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 64 deletions.
71 changes: 23 additions & 48 deletions charmcraft/charm_builder.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,62 +251,37 @@ def _install_dependencies(self, staging_venv_dir):
self._install_strict_dependencies(pip_cmd)
return

# Legacy non-strict dependencies.
# This method is not valid for any bases added after 2024-01-01 or for DEVEL bases.
try:
# Non-strict dependency resolution:
# 1. Install binary-allowed packages
# 2. Install source packages
# 3. Install from requirements files and charm libs dependencies
if self.binary_python_packages:
print(
"Installing binary-allowed packages and their dependencies.\n"
"WARNING: dependencies may also be installed from binary wheels.\n"
"Use strict mode to avoid these issues."
)
_process_run(
get_pip_command(
[pip_cmd, "install"],
self.requirement_paths,
source_deps=[*self.python_packages, *self.charmlib_deps],
requirements_files=[],
binary_deps=self.binary_python_packages,
)
)
except RuntimeError:
print(
"WARNING: Initial package installation failed. "
"Falling back to older method, which may leave your charm "
"in an un-runnable state."
)
if self.binary_python_packages:
# install python packages, allowing binary packages
cmd = [pip_cmd, "install", "--upgrade"] # base command
cmd.extend(self.binary_python_packages) # the python packages to install
_process_run(cmd)
if self.python_packages:
# install python packages from source
cmd = [
pip_cmd,
"install",
"--upgrade",
"--no-binary",
":all:",
] # base command
cmd.extend(self.python_packages) # the python packages to install
_process_run(cmd)
if self.requirement_paths:
# install dependencies from requirement files
cmd = [
pip_cmd,
"install",
"--upgrade",
"--no-binary",
":all:",
] # base command
for reqspath in self.requirement_paths:
cmd.append(f"--requirement={reqspath}") # the dependencies file(s)
_process_run(cmd)
if self.charmlib_deps:
# install charmlibs python dependencies
cmd = [
if self.python_packages:
print("Installing Python pre-dependencies from source.")
_process_run([pip_cmd, "install", "--no-binary=:all:", *self.python_packages])
if self.requirement_paths or self.charmlib_deps:
print("Installing packages from requirements files and charm lib dependencies.")
_process_run(
[
pip_cmd,
"install",
"--upgrade",
"--no-binary",
":all:",
] # base command
cmd.extend(self.charmlib_deps) # the python packages to install
_process_run(cmd)
"--no-binary=:all:",
*(f"--requirement={path}" for path in self.requirement_paths),
*self.charmlib_deps,
]
)

def _install_strict_dependencies(self, pip_cmd: str) -> None:
if not self.requirement_paths:
Expand Down
57 changes: 41 additions & 16 deletions tests/test_charm_builder.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -788,17 +788,15 @@ def test_build_dependencies_virtualenv_all(tmp_path, assert_output):
assert mock.mock_calls == [
call(["python3", "-m", "venv", str(tmp_path / const.STAGING_VENV_DIRNAME)]),
call([pip_cmd, "install", f"pip@{KNOWN_GOOD_PIP_URL}"]),
call([pip_cmd, "install", "pkg1", "pkg2"]),
call([pip_cmd, "install", "--no-binary=:all:", "pkg3", "pkg4"]),
call(
[
pip_cmd,
"install",
"--no-binary=pkg3,pkg4,pkg5,pkg6",
"--no-binary=:all:",
f"--requirement={reqs_file_1}",
f"--requirement={reqs_file_2}",
"pkg1",
"pkg2",
"pkg3",
"pkg4",
"pkg5",
"pkg6",
]
Expand Down Expand Up @@ -835,7 +833,9 @@ def test_build_dependencies_no_reused_missing_venv(tmp_path, assert_output):
with patch("shutil.copytree") as mock_copytree:
builder.handle_dependencies()
assert_output(
"Handling dependencies", "Dependencies directory not found", "Installing dependencies"
"Handling dependencies",
"Dependencies directory not found",
"Installing dependencies",
)

# directory created and packages installed
Expand All @@ -854,7 +854,9 @@ def test_build_dependencies_no_reused_missing_venv(tmp_path, assert_output):
with patch("shutil.copytree") as mock_copytree:
builder.handle_dependencies()
assert_output(
"Handling dependencies", "Dependencies directory not found", "Installing dependencies"
"Handling dependencies",
"Dependencies directory not found",
"Installing dependencies",
)

# directory created and packages installed *again*
Expand Down Expand Up @@ -890,7 +892,9 @@ def test_build_dependencies_no_reused_missing_hash_file(tmp_path, assert_output)
with patch("shutil.copytree") as mock_copytree:
builder.handle_dependencies()
assert_output(
"Handling dependencies", "Dependencies directory not found", "Installing dependencies"
"Handling dependencies",
"Dependencies directory not found",
"Installing dependencies",
)

# directory created and packages installed
Expand All @@ -909,7 +913,9 @@ def test_build_dependencies_no_reused_missing_hash_file(tmp_path, assert_output)
with patch("shutil.copytree") as mock_copytree:
builder.handle_dependencies()
assert_output(
"Handling dependencies", "Dependencies hash file not found", "Installing dependencies"
"Handling dependencies",
"Dependencies hash file not found",
"Installing dependencies",
)

# directory created and packages installed *again*
Expand Down Expand Up @@ -945,7 +951,9 @@ def test_build_dependencies_no_reused_problematic_hash_file(tmp_path, assert_out
with patch("shutil.copytree") as mock_copytree:
builder.handle_dependencies()
assert_output(
"Handling dependencies", "Dependencies directory not found", "Installing dependencies"
"Handling dependencies",
"Dependencies directory not found",
"Installing dependencies",
)

# directory created and packages installed
Expand Down Expand Up @@ -990,7 +998,12 @@ def test_build_dependencies_no_reused_problematic_hash_file(tmp_path, assert_out
],
)
def test_build_dependencies_no_reused_different_dependencies(
tmp_path, assert_output, new_reqs_content, new_pypackages, new_pybinaries, new_charmlibdeps
tmp_path,
assert_output,
new_reqs_content,
new_pypackages,
new_pybinaries,
new_charmlibdeps,
):
"""Dependencies are built again because changed from previous run."""
build_dir = tmp_path / const.BUILD_DIRNAME
Expand Down Expand Up @@ -1023,7 +1036,9 @@ def test_build_dependencies_no_reused_different_dependencies(
with patch("shutil.copytree") as mock_copytree:
builder.handle_dependencies()
assert_output(
"Handling dependencies", "Dependencies directory not found", "Installing dependencies"
"Handling dependencies",
"Dependencies directory not found",
"Installing dependencies",
)

# directory created and packages installed
Expand Down Expand Up @@ -1090,7 +1105,9 @@ def test_build_dependencies_reused(tmp_path, assert_output):
with patch("shutil.copytree") as mock_copytree:
builder.handle_dependencies()
assert_output(
"Handling dependencies", "Dependencies directory not found", "Installing dependencies"
"Handling dependencies",
"Dependencies directory not found",
"Installing dependencies",
)

# directory created and packages installed
Expand All @@ -1106,7 +1123,8 @@ def test_build_dependencies_reused(tmp_path, assert_output):
with patch("shutil.copytree") as mock_copytree:
builder.handle_dependencies()
assert_output(
"Handling dependencies", "Reusing installed dependencies, they are equal to last run ones"
"Handling dependencies",
"Reusing installed dependencies, they are equal to last run ones",
)

# installation directory copied *again* to the build directory (this is always done as
Expand Down Expand Up @@ -1188,7 +1206,10 @@ def mock_build_charm(self):
assert self.builddir == pathlib.Path("builddir")
assert self.installdir == pathlib.Path("installdir")
assert self.entrypoint == pathlib.Path("src/charm.py")
assert self.requirement_paths == [pathlib.Path("reqs1.txt"), pathlib.Path("reqs2.txt")]
assert self.requirement_paths == [
pathlib.Path("reqs1.txt"),
pathlib.Path("reqs2.txt"),
]
sys.exit(42)

fake_argv = ["cmd", "--builddir", "builddir", "--installdir", "installdir"]
Expand Down Expand Up @@ -1293,7 +1314,11 @@ def test_find_venv_site_packages(monkeypatch, platform, result):
site_packages_dir = charm_builder._find_venv_site_packages(basedir)
assert mock_run.mock_calls == [
call(
["python3", "-c", "import sys; v=sys.version_info; print(f'{v.major} {v.minor}')"],
[
"python3",
"-c",
"import sys; v=sys.version_info; print(f'{v.major} {v.minor}')",
],
text=True,
)
]
Expand Down

0 comments on commit 51a55f7

Please sign in to comment.