Skip to content

Commit

Permalink
fix(pip): set --no-binary=:all: if possible (#1740)
Browse files Browse the repository at this point in the history
This reduces the chances of having pip install binary packages as
indirect dependencies.

Partial fix for #1473
  • Loading branch information
lengau authored Jul 18, 2024
1 parent 5e119c3 commit 55434e0
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 21 deletions.
20 changes: 12 additions & 8 deletions charmcraft/utils/package.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,21 +103,25 @@ def get_pip_command(
binary_packages = get_pypi_packages(binary_deps)
requirements_packages = get_requirements_file_package_names(*requirements_files)
all_packages = charm_packages | binary_packages | requirements_packages
source_only_packages = sorted(
get_package_names(all_packages) - get_package_names(binary_packages)
)

non_requirements_packages = sorted(
exclude_packages(
set(source_deps) | set(binary_deps),
excluded=get_package_names(requirements_packages),
)
)

if source_only_packages:
no_binary = [f"--no-binary={','.join(source_only_packages)}"]
else:
no_binary = []
if not binary_packages:
return [
*prefix,
"--no-binary=:all:",
*(f"--requirement={path}" for path in requirements_files),
*non_requirements_packages,
]

source_only_packages = sorted(
get_package_names(all_packages) - get_package_names(binary_packages)
)
no_binary = [f"--no-binary={','.join(source_only_packages)}"] if source_only_packages else ()

return [
*prefix,
Expand Down
5 changes: 3 additions & 2 deletions tests/test_charm_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ def test_build_dependencies_virtualenv_simple(tmp_path, assert_output):
assert mock.mock_calls == [
call(["python3", "-m", "venv", str(tmp_path / STAGING_VENV_DIRNAME)]),
call([pip_cmd, "install", f"pip@{KNOWN_GOOD_PIP_URL}"]),
call([pip_cmd, "install", f"--requirement={reqs_file}"]),
call([pip_cmd, "install", "--no-binary=:all:", f"--requirement={reqs_file}"]),
]

site_packages_dir = charm_builder._find_venv_site_packages(pathlib.Path(STAGING_VENV_DIRNAME))
Expand Down Expand Up @@ -657,6 +657,7 @@ def test_build_dependencies_virtualenv_multiple(tmp_path, assert_output):
[
pip_cmd,
"install",
"--no-binary=:all:",
f"--requirement={reqs_file_1}",
f"--requirement={reqs_file_2}",
]
Expand Down Expand Up @@ -714,7 +715,7 @@ def test_build_dependencies_virtualenv_packages(tmp_path, assert_output):
assert mock.mock_calls == [
call(["python3", "-m", "venv", str(tmp_path / STAGING_VENV_DIRNAME)]),
call([pip_cmd, "install", f"pip@{KNOWN_GOOD_PIP_URL}"]),
call([pip_cmd, "install", "--no-binary=pkg1,pkg2", "pkg1", "pkg2"]),
call([pip_cmd, "install", "--no-binary=:all:", "pkg1", "pkg2"]),
]

site_packages_dir = charm_builder._find_venv_site_packages(pathlib.Path(STAGING_VENV_DIRNAME))
Expand Down
18 changes: 7 additions & 11 deletions tests/unit/test_charm_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,16 +98,12 @@ def test_install_strict_dependencies_success(
fs: FakeFilesystem, fake_process: FakeProcess, builder, requirements
):
fs.create_file("requirements.txt", contents=requirements)
no_binary_packages = utils.get_package_names(requirements.splitlines(keepends=False))
no_binary_packages_str = ",".join(sorted(no_binary_packages))
fake_process.register(
[
"/pip",
"install",
*([f"--no-binary={no_binary_packages_str}"] if no_binary_packages else []),
"--requirement=requirements.txt",
],
returncode=0,
)
expected_command = [
"/pip",
"install",
"--no-binary=:all:",
"--requirement=requirements.txt",
]
fake_process.register(expected_command, returncode=0)

builder._install_strict_dependencies("/pip")
2 changes: 2 additions & 0 deletions tests/unit/utils/test_package.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ def test_get_requirements_file_package_names(tmp_path, file_contents, expected):
"--no-binary=abc,ghi",
["ghi", "jkl"],
),
(["abc==1.0.0", "def>=1.2.3"], [], [], "--no-binary=:all:", []),
([], ["abc==1.0.0", "def>=1.2.3"], [], "--no-binary=:all:", ["abc==1.0.0", "def>=1.2.3"]),
],
)
@pytest.mark.parametrize("prefix", [["/bin/pip"], ["/some/path/to/pip3"], ["pip", "--some-param"]])
Expand Down

0 comments on commit 55434e0

Please sign in to comment.