Skip to content

Upgrade pip inside of any newly created pyenv #5195

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

Merged
pavera merged 3 commits into from
May 31, 2022
Merged

Upgrade pip inside of any newly created pyenv #5195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a36fb93
upgrade pip inside of new pyenvs first thing
pavera May 25, 2022
a8f732a
add upper version bound to pip and pip-tools
pavera May 26, 2022
bab6aed
Mark this test as slow to see how build times do.
pavera May 26, 2022
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
4 changes: 2 additions & 2 deletions python/helpers/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
pip>=21.3.1 # Allow earlier versions to retain python 3.6 support
pip-tools>=6.4.0 # Allow earlier versions to retain python 3.6 support
pip>=21.3.1,<=22.1.1 # Allow earlier versions to retain python 3.6 support
pip-tools>=6.4.0,<=6.6.2 # Allow earlier versions to retain python 3.6 support
Copy link
Copy Markdown
Member

@jeffwidman jeffwidman May 26, 2022

Choose a reason for hiding this comment

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

Why pin the max version? Or more specifically, why these particular versions for the max allowed?

Ideally this would be a code comment so that someone who comes along later and wonders if it's safe to bump the max allowed version will have more context w/o having to spelunk through git blame.

Copy link
Copy Markdown
Contributor Author

@pavera pavera May 27, 2022

Choose a reason for hiding this comment

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

This was mostly to break docker build cache so it would get the latest version of pip-tools, but also this will cause dependabot to manage these dependencies again. Either way, I'm happy to revert this change as it was just to force a rebuild of the helpers.

Copy link
Copy Markdown
Member

@jeffwidman jeffwidman May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

also this will cause dependabot to manage these dependencies again.

Pinning makes sense to me for this reason alone, although I'd probably pin a single concrete version rather than a range just for more determinism.

Although on second thought, we still get the determinism if we don't pin at all, we just get whatever pip was shipped with that version of python.

That said, I'm fine either way.

Copy link
Copy Markdown
Contributor Author

@pavera pavera May 27, 2022

Choose a reason for hiding this comment

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

Yeah, the ranges are here again to support earlier python versions. The latest pip and pip-tools have removed support for python 3.6, so if we just run latest, we can't support 3.6. We could pin to the earlier version, but then we have to make sure not to merge any dependabot PRs which will try to pull the pin forward...

Copy link
Copy Markdown
Member

@jeffwidman jeffwidman May 27, 2022

Choose a reason for hiding this comment

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

Oh right, that makes complete sense to keep the range.

flake8==4.0.1
hashin==0.17.0
pipenv==2022.4.8
Expand Down
1 change: 1 addition & 0 deletions python/lib/dependabot/python/file_updater/pip_compile_file_updater.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ def install_required_python
return if run_command("pyenv versions").include?("#{python_version}\n")

run_command("pyenv install -s #{python_version}")
run_command("pyenv exec pip install --upgrade pip")
Copy link
Copy Markdown
Member

@jeffwidman jeffwidman May 27, 2022

Choose a reason for hiding this comment

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

Just so I understand how this works... you're first manually saying grab the latest version of pip here, and then in the subsequent command is where it will revert back to a specific pip version?

The original reason for this PR was a broken pip version, so with this current code we could still end up installing a broken pip in the first step, before trying to revert it... but you decided not to pin here because then suddenly we're hardcoding versions within the code rather than the lockfile?
And if a new version of pip ships that's broken, we can temp pin here and then revert once the latest version is unbroken?

I'm totally fine with the route you went, just want to confirm my understanding is correct.

Copy link
Copy Markdown
Contributor Author

@pavera pavera May 27, 2022

Choose a reason for hiding this comment

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

Yes, that is all correct. With the additional detail that when pip ships a broken version (they do this at least a couple times a year in my experience), it probably won't be broken for a basic install like the second install here, so we should always be able to revert to the pinned version in the lock file.

run_command("pyenv exec pip install -r "\
"#{NativeHelpers.python_requirements_path}")
end
Expand Down
1 change: 1 addition & 0 deletions python/lib/dependabot/python/file_updater/poetry_file_updater.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ def updated_lockfile_content_for(pyproject_content)

if python_version && !pre_installed_python?(python_version)
run_poetry_command("pyenv install -s #{python_version}")
run_poetry_command("pyenv exec pip install --upgrade pip")
run_poetry_command("pyenv exec pip install -r"\
"#{NativeHelpers.python_requirements_path}")
end
Expand Down
1 change: 1 addition & 0 deletions python/lib/dependabot/python/update_checker/pip_compile_version_resolver.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ def install_required_python
return if run_command("pyenv versions").include?("#{python_version}\n")

run_command("pyenv install -s #{python_version}")
run_command("pyenv exec pip install --upgrade pip")
run_command("pyenv exec pip install -r"\
"#{NativeHelpers.python_requirements_path}")
end
Expand Down
1 change: 1 addition & 0 deletions python/lib/dependabot/python/update_checker/pipenv_version_resolver.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ def install_required_python

requirements_path = NativeHelpers.python_requirements_path
run_command("pyenv install -s #{python_version}")
run_command("pyenv exec pip install --upgrade pip")
run_command("pyenv exec pip install -r "\
"#{requirements_path}")
end
Expand Down
1 change: 1 addition & 0 deletions python/lib/dependabot/python/update_checker/poetry_version_resolver.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ def fetch_latest_resolvable_version_string(requirement:)

if python_version && !pre_installed_python?(python_version)
run_poetry_command("pyenv install -s #{python_version}")
run_poetry_command("pyenv exec pip install --upgrade pip")
run_poetry_command(
"pyenv exec pip install -r "\
"#{NativeHelpers.python_requirements_path}"
Expand Down
2 changes: 1 addition & 1 deletion python/spec/dependabot/python/file_updater/poetry_file_updater_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@
end
end

context "with a supported python version" do
context "with a supported python version", :slow do
let(:python_version) { "3.6.9" }
let(:pyproject_fixture_name) { "python_36.toml" }
let(:lockfile_fixture_name) { "python_36.lock" }
Expand Down