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.

Sign up for GitHub

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

Remove --upgrade from pip install #1136

Closed
wants to merge 4 commits into from
Closed

Remove --upgrade from pip install #1136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97d25d3
Remove `--upgrade` from `pip install`
carlcsaposs-canonical Jul 3, 2023
6373de6
format test
carlcsaposs-canonical Jul 3, 2023
a4812c6
Add spread test
carlcsaposs-canonical Jul 5, 2023
de9060b
Update task.yaml
carlcsaposs-canonical Jul 5, 2023
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
8 changes: 4 additions & 4 deletions charmcraft/charm_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,26 +239,26 @@ def _install_dependencies(self, staging_venv_dir):
with instrum.Timer("Installing all dependencies"):
if self.binary_python_packages:
# install python packages, allowing binary packages
cmd = [pip_cmd, "install", "--upgrade"] # base command
cmd = [pip_cmd, "install"] # base command
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is breaking centos 7. I'm not sure what the best way to handle it is, so I'm going to tag in @syu-w for thoughts. My best proposal right now is to add a pip install --upgrade pip setuptools run before starting any of the conditional installs.

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 = [pip_cmd, "install", "--no-binary", ":all:"] # base command
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the Jupyter notebooks charm is failing because of this (see spread test.

Between this and the above, I'm wondering whether the right change for now might be to keep the --upgrade here and above and merge the two commands below (given the edge case we discussed in the main conversation).

I believe that'll resolve #1135 with a fairly minimal change that we can put into a 2.4 release fairly quickly, with a bigger rethink of package handling for a future release. How's that sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we can change the order of install requirement.txt and PYDEPS.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if a pinned version is installed, pip install without --upgrade will not try to get a new version. And if the package is not in requirement.txt, it will install latest stable version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was my original thought, but it could leave us with invalid PYDEPS that we won't discover until runtime.

Example:

PYDEPS includes pkg_a>=2.0, and requirement.txt includes pkg_a==1.0

Ideal behaviour would probably be to error out during the charm build due to a dependency conflict. However:

  • Current behaviour: we get pkg_a>=2.0 and a maybe-working charm.
  • Alternative behaviour: we get pkg_a==1.0 and an almost-guaranteed-broken charm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we need to check all deps in charmcraft without rely on pip

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe we can add PYDEPS to a new requirement.txt

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
cmd = [pip_cmd, "install", "--no-binary", ":all:"] # base command
for reqspath in self.requirement_paths:
cmd.append("--requirement={}".format(reqspath)) # the dependencies file(s)
_process_run(cmd)

if self.charmlib_deps:
# install charmlibs python dependencies
cmd = [pip_cmd, "install", "--upgrade", "--no-binary", ":all:"] # base command
cmd = [pip_cmd, "install", "--no-binary", ":all:"] # base command
cmd.extend(self.charmlib_deps) # the python packages to install
_process_run(cmd)

Expand Down
38 changes: 38 additions & 0 deletions tests/spread/smoketests/pinned-dependencies/task.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
summary: pack a charm with a specific (old) dependency version

include:
- tests/

prepare: |
tests.pkgs install unzip
charmcraft init --project-dir=charm
cd charm
echo "ops==2.0.0" > requirements.txt

cat <<- EOF >> charmcraft.yaml
parts:
charm:
charm-requirements: [requirements.txt]
EOF

mkdir -p lib/charms/charm/v0/
cat <<- EOF > lib/charms/charm/v0/my_lib.py
PYDEPS = ["ops"]
LIBID = "my_lib"
LIBAPI = 0
LIBPATCH = 1
EOF


restore: |
pushd charm
charmcraft clean
popd

rm -rf charm

execute: |
cd charm
charmcraft pack --verbose
test -f charm*.charm
unzip -p charm_*.charm venv/ops/version.py | MATCH "version = '2.0.0'"
16 changes: 6 additions & 10 deletions tests/test_charm_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,9 +613,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, "--version"]),
call(
[pip_cmd, "install", "--upgrade", "--no-binary", ":all:", 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 @@ -654,7 +652,6 @@ def test_build_dependencies_virtualenv_multiple(tmp_path, assert_output):
[
pip_cmd,
"install",
"--upgrade",
"--no-binary",
":all:",
f"--requirement={reqs_file_1}",
Expand Down Expand Up @@ -712,7 +709,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, "--version"]),
call([pip_cmd, "install", "--upgrade", "--no-binary", ":all:", "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 Expand Up @@ -743,7 +740,7 @@ def test_build_dependencies_virtualenv_binary_packages(tmp_path, assert_output):
assert mock.mock_calls == [
call(["python3", "-m", "venv", str(tmp_path / STAGING_VENV_DIRNAME)]),
call([pip_cmd, "--version"]),
call([pip_cmd, "install", "--upgrade", "pkg1", "pkg2"]),
call([pip_cmd, "install", "pkg1", "pkg2"]),
]

site_packages_dir = charm_builder._find_venv_site_packages(pathlib.Path(STAGING_VENV_DIRNAME))
Expand Down Expand Up @@ -780,20 +777,19 @@ def test_build_dependencies_virtualenv_all(tmp_path, assert_output):
assert mock.mock_calls == [
call(["python3", "-m", "venv", str(tmp_path / STAGING_VENV_DIRNAME)]),
call([pip_cmd, "--version"]),
call([pip_cmd, "install", "--upgrade", "pkg1", "pkg2"]),
call([pip_cmd, "install", "--upgrade", "--no-binary", ":all:", "pkg3", "pkg4"]),
call([pip_cmd, "install", "pkg1", "pkg2"]),
call([pip_cmd, "install", "--no-binary", ":all:", "pkg3", "pkg4"]),
call(
[
pip_cmd,
"install",
"--upgrade",
"--no-binary",
":all:",
f"--requirement={reqs_file_1}",
f"--requirement={reqs_file_2}",
]
),
call([pip_cmd, "install", "--upgrade", "--no-binary", ":all:", "pkg5", "pkg6"]),
call([pip_cmd, "install", "--no-binary", ":all:", "pkg5", "pkg6"]),
]

site_packages_dir = charm_builder._find_venv_site_packages(pathlib.Path(STAGING_VENV_DIRNAME))
Expand Down