Skip to content

Require the command in uvx <name> to be available in the Python environment #11603

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
zanieb merged 4 commits into from
Apr 18, 2025
Merged

Require the command in uvx <name> to be available in the Python environment #11603

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
ff5b6b5
Improve executable checks in `uvx`
zanieb Apr 18, 2025
d5cab42
Fix construction
zanieb Apr 18, 2025
a7c86bb
Review
zanieb Apr 18, 2025
5456abd
Fix rebase
zanieb Apr 18, 2025
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
286 changes: 169 additions & 117 deletions crates/uv/src/commands/tool/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use uv_cli::ExternalCommand;
use uv_client::BaseClientBuilder;
use uv_configuration::Constraints;
use uv_configuration::{Concurrency, PreviewMode};
use uv_distribution_types::InstalledDist;
use uv_distribution_types::{
IndexUrl, Name, NameRequirementSpecification, Requirement, RequirementSource,
UnresolvedRequirement, UnresolvedRequirementSpecification,
Expand All @@ -39,6 +40,7 @@ use uv_shell::runnable::WindowsRunnable;
use uv_static::EnvVars;
use uv_tool::{entrypoint_paths, InstalledTools};
use uv_warnings::warn_user;
use uv_warnings::warn_user_once;
use uv_workspace::WorkspaceCache;

use crate::commands::pip::loggers::{
Expand Down Expand Up @@ -270,6 +272,7 @@ pub(crate) async fn run(
))
.await;

let explicit_from = from.is_some();
let (from, environment) = match result {
Ok(resolution) => resolution,
Err(ProjectError::Operation(err)) => {
Expand Down Expand Up @@ -304,6 +307,38 @@ pub(crate) async fn run(

// TODO(zanieb): Determine the executable command via the package entry points
let executable = from.executable();
let site_packages = SitePackages::from_environment(&environment)?;

// Check if the provided command is not part of the executables for the `from` package,
// and if it's provided by another package in the environment.
let provider_hints = match &from {
ToolRequirement::Python => None,
ToolRequirement::Package { requirement, .. } => Some(ExecutableProviderHints::new(
executable,
requirement,
&site_packages,
invocation_source,
)),
};

if let Some(ref provider_hints) = provider_hints {
if provider_hints.not_from_any() {
if !explicit_from {
// If the user didn't use `--from` and the command isn't in the environment, we're now
// just invoking an arbitrary executable on the `PATH` and should exit instead.
writeln!(printer.stderr(), "{provider_hints}")?;
return Ok(ExitStatus::Failure);
}
// In the case where `--from` is used, we'll warn on failure if the command is not found
// TODO(zanieb): Consider if we should require `--with` instead of `--from` in this case?
// It'd be a breaking change but would make `uvx` invocations safer.
} else if provider_hints.not_from_expected() {
// However, if the user used `--from`, we shouldn't fail because they requested that the
// package and executable be different. We'll warn if the executable comes from another
// package though, because that could be confusing
warn_user_once!("{provider_hints}");
}
}

// Construct the command
let mut process = if cfg!(windows) {
Expand All @@ -327,43 +362,24 @@ pub(crate) async fn run(

// Spawn and wait for completion
// Standard input, output, and error streams are all inherited
// TODO(zanieb): Throw a nicer error message if the command is not found
let space = if args.is_empty() { "" } else { " " };
debug!(
"Running `{}{space}{}`",
executable,
args.iter().map(|arg| arg.to_string_lossy()).join(" ")
);

let site_packages = SitePackages::from_environment(&environment)?;

// We check if the provided command is not part of the executables for the `from` package.
// If the command is found in other packages, we warn the user about the correct package to use.
match &from {
ToolRequirement::Python => {}
ToolRequirement::Package {
requirement: from, ..
} => {
warn_executable_not_provided_by_package(
executable,
&from.name,
&site_packages,
invocation_source,
);
}
}

let handle = match process.spawn() {
Ok(handle) => Ok(handle),
Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
if let Some(exit_status) = hint_on_not_found(
executable,
&from,
&site_packages,
invocation_source,
printer,
)? {
return Ok(exit_status);
if let Some(ref provider_hints) = provider_hints {
if provider_hints.not_from_any() && explicit_from {
// We deferred this warning earlier, because `--from` was used and the command
// could have come from the `PATH`. Display a more helpful message instead of the
// OS error.
writeln!(printer.stderr(), "{provider_hints}")?;
return Ok(ExitStatus::Failure);
}
}
Err(err)
}
Expand All @@ -374,67 +390,6 @@ pub(crate) async fn run(
run_to_completion(handle).await
}

/// Show a hint when a command fails due to a missing executable.
///
/// Returns an exit status if the caller should exit after hinting.
fn hint_on_not_found(
executable: &str,
from: &ToolRequirement,
site_packages: &SitePackages,
invocation_source: ToolRunCommand,
printer: Printer,
) -> anyhow::Result<Option<ExitStatus>> {
let from = match from {
ToolRequirement::Python => return Ok(None),
ToolRequirement::Package {
requirement: from, ..
} => from,
};
match get_entrypoints(&from.name, site_packages) {
Ok(entrypoints) => {
writeln!(
printer.stdout(),
"The executable `{}` was not found.",
executable.cyan(),
)?;
if entrypoints.is_empty() {
warn_user!(
"Package `{}` does not provide any executables.",
from.name.red()
);
} else {
warn_user!(
"An executable named `{}` is not provided by package `{}`.",
executable.cyan(),
from.name.red()
);
writeln!(
printer.stdout(),
"The following executables are provided by `{}`:",
from.name.green()
)?;
for (name, _) in entrypoints {
writeln!(printer.stdout(), "- {}", name.cyan())?;
}
let suggested_command = format!(
"{} --from {} <EXECUTABLE_NAME>",
invocation_source, from.name
);
writeln!(
printer.stdout(),
"Consider using `{}` instead.",
suggested_command.green()
)?;
}
Ok(Some(ExitStatus::Failure))
}
Err(err) => {
warn!("Failed to get entrypoints for `{from}`: {err}");
Ok(None)
}
}
}

/// Return the entry points for the specified package.
fn get_entrypoints(
from: &PackageName,
Expand Down Expand Up @@ -517,52 +472,149 @@ async fn show_help(
Ok(())
}

/// Display a warning if an executable is not provided by package.
///
/// If found in a dependency of the requested package instead of the requested package itself, we will hint to use that instead.
fn warn_executable_not_provided_by_package(
executable: &str,
from_package: &PackageName,
site_packages: &SitePackages,
/// A set of hints about the packages that provide an executable.
#[derive(Debug)]
struct ExecutableProviderHints<'a> {
/// The requested executable for the command
executable: &'a str,
/// The package from which the executable is expected to come from
from: &'a Requirement,
/// The packages in the [`PythonEnvironment`] the command will run in
site_packages: &'a SitePackages,
/// The packages with matching executable names
packages: Vec<InstalledDist>,
/// The source of the invocation, for suggestions to the user
invocation_source: ToolRunCommand,
) {
let packages = matching_packages(executable, site_packages);
if !packages
.iter()
.any(|package| package.name() == from_package)
{
}

impl<'a> ExecutableProviderHints<'a> {
fn new(
executable: &'a str,
from: &'a Requirement,
site_packages: &'a SitePackages,
invocation_source: ToolRunCommand,
) -> Self {
let packages = matching_packages(executable, site_packages);
ExecutableProviderHints {
executable,
from,
site_packages,
packages,
invocation_source,
}
}

/// If the executable is not provided by the expected package.
fn not_from_expected(&self) -> bool {
!self
.packages
.iter()
.any(|package| package.name() == &self.from.name)
}

/// If the executable is not provided by any package.
fn not_from_any(&self) -> bool {
self.packages.is_empty()
}
}

impl std::fmt::Display for ExecutableProviderHints<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Copy link
Contributor

@jtfmumm jtfmumm Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems surprising that Display doesn't display the representation of the type but warning and error information. Should this be a separate dedicated function or method?

Copy link
Member Author

@zanieb zanieb Apr 18, 2025

Choose a reason for hiding this comment

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

I'm not sure, the type only exists for the purpose of rendering these messages. Previously, it was a function that called warn_user and println directly but here I want the caller to be able to make that decision and I think Display is the easiest way to do that. I think I'd just end up needing into introduce a separate type, like ExecutableProviderPackagesDisplay that implements Display? Or I'd need to implement like fn message(&self)-> String and construct a string instead of using Formatter?

Copy link
Contributor

@jtfmumm jtfmumm Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was confused when I encountered writeln!(printer.stderr(), "{providers}")?; above (before seeing this code). I didn't understand why you would just log the providers without context. It seems pretty non-standard to have Display perform conditional logic and log warnings.

I'd recommend something like providers.log_warnings() (or something more descriptive) to accomplish what you're doing here. You could pass in printer.

Copy link
Member Author

@zanieb zanieb Apr 18, 2025

Choose a reason for hiding this comment

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

perform conditional logic and log warnings.

To be clear, it only logs one warning (which we could omit, but I don't think it really hurts — it's a case that should never happen). The rest of it is just writing to the formatter.

I can't use a function that takes a printer because I want to be able to use it in warn_user_once.

I see this as the same thing as implementing Display on an error type, which we frequently include more context on. Would it be clearer if it was named like ExecutableProviderHints instead?

Copy link
Contributor

@jtfmumm jtfmumm Apr 18, 2025

Choose a reason for hiding this comment

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

Yeah that would be clearer

Copy link
Member Author

@zanieb zanieb Apr 18, 2025

Choose a reason for hiding this comment

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

That makes sense, thanks!

let Self {
executable,
from,
site_packages,
packages,
invocation_source,
} = self;

match packages.as_slice() {
[] => {}
[] => {
let entrypoints = match get_entrypoints(&from.name, site_packages) {
Ok(entrypoints) => entrypoints,
Err(err) => {
warn!("Failed to get entrypoints for `{from}`: {err}");
return Ok(());
}
};
if entrypoints.is_empty() {
write!(
f,
"Package `{}` does not provide any executables.",
from.name.red()
)?;
return Ok(());
}
writeln!(
f,
"An executable named `{}` is not provided by package `{}`.",
executable.cyan(),
from.name.cyan(),
)?;
writeln!(f, "The following executables are available:")?;
for (name, _) in &entrypoints {
writeln!(f, "- {}", name.cyan())?;
}
let name = match entrypoints.as_slice() {
[entrypoint] => entrypoint.0.as_str(),
_ => "<EXECUTABLE-NAME>",
};
// If the user didn't use `--from`, suggest it
if *executable == from.name.as_str() {
let suggested_command =
format!("{} --from {} {name}", invocation_source, from.name);
writeln!(f, "\nUse `{}` instead.", suggested_command.green().bold())?;
}
}
[package] if package.name() == &from.name => {
write!(
f,
"An executable named `{}` is provided by package `{}`",
executable.cyan(),
from.name.cyan(),
)?;
}
[package] => {
let suggested_command = format!(
"{invocation_source} --from {} {}",
package.name(),
executable
);
warn_user!(
"An executable named `{}` is not provided by package `{}` but is available via the dependency `{}`. Consider using `{}` instead.",
executable.cyan(),
from_package.cyan(),
package.name().cyan(),
suggested_command.green()
);
write!(f,
"An executable named `{}` is not provided by package `{}` but is available via the dependency `{}`. Consider using `{}` instead.",
executable.cyan(),
from.name.cyan(),
package.name().cyan(),
suggested_command.green()
)?;
}
packages => {
let suggested_command = format!("{invocation_source} --from PKG {executable}");
let provided_by = packages
.iter()
.map(uv_distribution_types::Name::name)
.map(|name| format!("- {}", name.cyan()))
.join("\n");
warn_user!(
"An executable named `{}` is not provided by package `{}` but is available via the following dependencies:\n- {}\nConsider using `{}` instead.",
executable.cyan(),
from_package.cyan(),
provided_by,
suggested_command.green(),
);
if self.not_from_expected() {
let suggested_command = format!("{invocation_source} --from PKG {executable}");
write!(f,
"An executable named `{}` is not provided by package `{}` but is available via the following dependencies:\n- {}\nConsider using `{}` instead.",
executable.cyan(),
from.name.cyan(),
provided_by,
suggested_command.green(),
)?;
} else {
write!(f,
"An executable named `{}` is provided by package `{}` but is also available via the following dependencies:\n- {}\nUnexpected behavior may occur.",
executable.cyan(),
from.name.cyan(),
provided_by,
)?;
}
}
}

Ok(())
}
}

Expand Down
Loading
Loading