Skip to content

Prevent installation failure with custom install_package using pip#210

Closed
ssbarnea wants to merge 1 commit intotox-dev:mainfrom
ssbarnea:fix/install_package
Closed

Prevent installation failure with custom install_package using pip#210
ssbarnea wants to merge 1 commit intotox-dev:mainfrom
ssbarnea:fix/install_package

Conversation

@ssbarnea
Copy link
Copy Markdown
Member

@ssbarnea ssbarnea commented Jun 12, 2025

As some users might still use install_package and point to original pip, we want to use correct arguments names to this one.

@ssbarnea ssbarnea requested a review from gaborbernat as a code owner June 12, 2025 16:31
@ssbarnea ssbarnea marked this pull request as draft June 12, 2025 16:31
@ssbarnea ssbarnea force-pushed the fix/install_package branch from e1a9586 to 7ebff3b Compare June 12, 2025 18:58
@ssbarnea ssbarnea marked this pull request as ready for review June 12, 2025 18:59
@webknjaz
Copy link
Copy Markdown

Nice! This should be able to address my pain with defaulting to tox-uv in reusable-tox.yml where I also have custom env pinning implemented. If only the PR didn't bundle unrelated changes, though…

@ssbarnea ssbarnea force-pushed the fix/install_package branch from 7ebff3b to 991ab70 Compare June 20, 2025 10:24
@ssbarnea ssbarnea requested review from gaborbernat and webknjaz June 20, 2025 10:29
@webknjaz
Copy link
Copy Markdown

Can you add tests with only some envs having a custom install command?

@ssbarnea ssbarnea force-pushed the fix/install_package branch from 991ab70 to 51e8d40 Compare June 20, 2025 11:25
@ssbarnea
Copy link
Copy Markdown
Member Author

Can you add tests with only some envs having a custom install command?

Done. Modified that test to include all 3 cases.

if len(install_cmd) < 1 or not isinstance(install_cmd[0], str): # pragma: no cover
msg = f"Unable to determine install command. {install_cmd}"
raise RuntimeError(msg)
self._pkg_manager = "uv" if install_cmd[0].endswith("uv") else "pip"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd say custom install commands are not supported with tox-uv 🤔 I'd rather raise an error than support it..

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I disagree: people may want a different installer for some of the envs but not the others. Is there an alternative workaround? Can I force a different installer for a specific env?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why, what's the use case?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have own lock files integrated like this, for example. And when tox-uv is installed in the env, it interferes with everything and breaks stuff in the end.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please expand.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ssbarnea can you also explain the use case for this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I did have few isolated cases where I wanted to use pip instead of uv installer. In these cases it was because uv installer was buggy (or it could also be because the problematic package happened to install with pip and fail with uv for their own internal assumptions). I think that we should allow users to have custom installers to allow them to implement workarounds.

On the other hand, if you want I can change the rewrite logic to do nothing if the case is unable to detect either pip or uv as being inside the install_cmd.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did have few isolated cases where I wanted to use pip instead of uv installer. In these cases it was because uv installer was buggy (or it could also be because the problematic package happened to install with pip and fail with uv for their own internal assumptions).

In those cases you can just set the runner to be not the uv one, no?

As some users might still use install_package and point to original
pip, we want to use correct arguments names to this one.
@ssbarnea ssbarnea force-pushed the fix/install_package branch from 51e8d40 to 73d44d7 Compare June 23, 2025 11:29
@ssbarnea ssbarnea requested a review from gaborbernat June 23, 2025 14:30
@gaborbernat gaborbernat marked this pull request as draft June 23, 2025 14:55
@gaborbernat
Copy link
Copy Markdown
Member

In those cases you can just set the runner to be not the uv one, no?

@gaborbernat gaborbernat closed this Aug 7, 2025
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