Skip to content

fix(pipx): upgrade shared pip environment when using version constraints#10138

Merged
jdx merged 1 commit into
jdx:mainfrom
iloveitaly:pipx-version-upgrade
May 31, 2026
Merged

fix(pipx): upgrade shared pip environment when using version constraints#10138
jdx merged 1 commit into
jdx:mainfrom
iloveitaly:pipx-version-upgrade

Conversation

@iloveitaly
Copy link
Copy Markdown
Contributor

  • Trigger an upgrade of the shared pip environment to meet minimum requirements (pip>=26.0) when before_date constraints are applied.
  • Add an e2e regression test to verify installation flows for packages with minimum_release_age.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for upgrading the shared pip version in pipx when a release age restriction (before_date) is specified, ensuring compatibility with the --uploaded-prior-to flag. It also adds a corresponding regression test. The review feedback highlights two key improvements: handling potential failures gracefully during the shared pip upgrade to prevent hard installation failures, and ensuring the $HOME/bin directory is explicitly created in the test script before writing to it.

Comment thread src/backend/pipx.rs Outdated
Comment on lines +294 to +303
if ctx.before_date.is_some() {
Self::upgrade_shared_pip_for_uploaded_prior_to(
&ctx.config,
self,
&tv,
&ctx.ts,
ctx.pr.as_ref(),
)
.await?;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If pipx upgrade-shared fails (for example, if the user is running an older version of pipx that does not support the upgrade-shared command or --pip-args, or in offline/restricted environments), the entire installation will fail. Since the upgrade is a best-effort attempt to ensure pip>=26.0 for the --uploaded-prior-to flag, we should handle any errors gracefully by logging a debug message and proceeding with the installation rather than failing hard.

            if ctx.before_date.is_some() {
                if let Err(err) = Self::upgrade_shared_pip_for_uploaded_prior_to(
                    &ctx.config,
                    self,
                    &tv,
                    &ctx.ts,
                    ctx.pr.as_ref(),
                )
                .await
                {
                    debug!("failed to upgrade shared pip: {err:#}");
                }
            }

Comment on lines +8 to +9
# Create system "tools" that always fail and push them to the front of PATH
cat >"$HOME/bin/fail" <<'EOF'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The directory "$HOME/bin" may not exist in the test environment, which will cause the cat command to fail with a 'No such file or directory' error. Ensure the directory is created before writing the file.

# Create system "tools" that always fail and push them to the front of PATH
mkdir -p "$HOME/bin"
cat >"$HOME/bin/fail" <<'EOF'

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 29, 2026

Greptile Summary

This PR adds a single documentation line to docs/dev-tools/backends/pipx.md clarifying that pip >= 26.0 is required for the --uploaded-prior-to flag used by the pipx install path when minimum_release_age is set. The pip version requirement itself is factually correct — pip 26.0 (released January 2026) introduced --uploaded-prior-to.

  • The added sentence states users "are responsible for ensuring that compatible Python/pip environment is installed," but the PR title ("fix(pipx): upgrade shared pip environment") and description ("trigger an upgrade of the shared pip environment") imply mise should handle the upgrade automatically — a code change that is not present in this PR.
  • The PR description also mentions adding an e2e regression test, but no test file appears in the diff.

Confidence Score: 3/5

The only file changed is documentation, so there is no runtime regression risk — but the added sentence tells users they are responsible for pip upgrades while the PR title and description promise automatic upgrade behavior with no corresponding code change.

The documentation addition correctly states the pip version requirement, but directly contradicts the stated goal of the PR. If the intent is auto-upgrade, the code to do it is absent and the docs are backwards. If the intent is only to document the manual requirement, the PR title and description are misleading and should be corrected before merge.

docs/dev-tools/backends/pipx.md — the new sentence needs to be aligned with actual code behavior or the missing code change needs to be added.

Important Files Changed

Filename Overview
docs/dev-tools/backends/pipx.md Adds one-line note that pip >= 26.0 is required for --uploaded-prior-to support with the pipx path, but the wording ("you are responsible") contradicts the PR title/description which promises automatic pip upgrade behavior that has no corresponding code change.

Reviews (2): Last reviewed commit: "docs: clarify pip version requirement fo..." | Re-trigger Greptile

Comment thread src/backend/pipx.rs Outdated
Comment on lines +294 to +303
if ctx.before_date.is_some() {
Self::upgrade_shared_pip_for_uploaded_prior_to(
&ctx.config,
self,
&tv,
&ctx.ts,
ctx.pr.as_ref(),
)
.await?;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Redundant upgrade-shared on every install with before_date

upgrade-shared is invoked unconditionally each time a pipx package with before_date set is installed — no check is made whether the shared pip in that environment already satisfies pip>=26.0. On a mise install run with several packages that all use minimum_release_age, this executes one network-touching upgrade-pip subprocess per package even when pip was already upgraded in an earlier iteration. Wrapping the call in a lazy check (e.g. querying the installed pip version in the shared venv first) or short-circuiting after the first success within a single install run would avoid the redundant work.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@risu729
Copy link
Copy Markdown
Contributor

risu729 commented May 29, 2026

Why do we need this? I thought pipx creates a new pip environment every time. See #9190

Thanks for the PR! I didn't notice this case.

@risu729
Copy link
Copy Markdown
Contributor

risu729 commented May 29, 2026

Yes, my understanding in #9190 was incomplete.

mise does give pipx a fresh PIPX_HOME for a fresh tool install, so the shared env path is fresh too. The part I missed is that this does not guarantee the first pip process in that shared env already supports --uploaded-prior-to.

In pipx's pip backend, create_venv() calls shared_libs.create(verbose=..., pip_args=pip_args). shared_libs.create() creates the shared venv and then upgrades shared pip using the same pip_args from the current pipx install. So when mise passes --pip-args=--uploaded-prior-to=..., that flag can be used while bootstrapping/upgrading shared pip itself, before pip has been upgraded to >=26.0. If the ensurepip/bootstrap pip is older, it errors with no such option: --uploaded-prior-to before it can install a newer pip.

So the separate pipx upgrade-shared --pip-args pip>=26.0 step is useful: it initializes/upgrades the shared pip without passing --uploaded-prior-to, then the actual package install can safely use that flag.

This comment was generated by an AI coding assistant.

@iloveitaly iloveitaly force-pushed the pipx-version-upgrade branch from dff9ffc to 8b08623 Compare May 31, 2026 06:19
@jdx jdx merged commit c0230c8 into jdx:main May 31, 2026
33 checks passed
This was referenced May 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants