Skip to content

Return error when running uvx with a .py script #11623

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

Jump to bottom
Merged
zanieb merged 9 commits into from
Mar 4, 2025
Merged

Return error when running uvx with a .py script #11623

zanieb merged 9 commits into from
Mar 4, 2025

Conversation

@jtfmumm
Copy link
Contributor

@jtfmumm jtfmumm commented Feb 19, 2025

If we see uvx script.py, we exit early, giving a hint to use uv run script.py if the script exists. If it does not exist, we suggest running uv run with a normalized package name.

This PR includes a snapshot test for each of these scenarios.

An alternative approach would be to wait until we encounter an error, and then add the hint. But if there happens to be a malicious package called script-py, this would be run unintentionally (a point raised by @zanieb).

Closes #10784

zanieb
zanieb reviewed Feb 19, 2025
View reviewed changes
zanieb
zanieb reviewed Feb 19, 2025
View reviewed changes
zanieb
zanieb reviewed Feb 19, 2025
View reviewed changes
zanieb
zanieb reviewed Feb 19, 2025
View reviewed changes
crates/uv/src/commands/tool/run.rs Outdated
.extension()
.is_some_and(|ext| ext.eq_ignore_ascii_case("py"))
{
if possible_path.try_exists()? {
Copy link
Member

@zanieb zanieb Feb 19, 2025

Choose a reason for hiding this comment

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

Should we error if we can't read the path? (earnestly not sure here)

Copy link
Contributor Author

@jtfmumm jtfmumm Feb 19, 2025

Choose a reason for hiding this comment

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

It's a good question. We could treat that as the "doesn't exist case" if we'd prefer.

zanieb
zanieb reviewed Feb 19, 2025
View reviewed changes
zanieb
zanieb reviewed Feb 19, 2025
View reviewed changes
zanieb
zanieb reviewed Feb 19, 2025
View reviewed changes
zanieb
zanieb reviewed Feb 19, 2025
View reviewed changes
@jtfmumm jtfmumm assigned jtfmumm and unassigned jtfmumm Feb 20, 2025
@zanieb
Copy link
Member

zanieb commented Feb 20, 2025

This looks good to me, thanks for addressing the feedback. I want to do one final pass on the messaging, since this is ostensibly breaking (though I really hope nobody is relying on this behavior). I'll do that on Monday.

zanieb
zanieb reviewed Mar 3, 2025
View reviewed changes
Copy link
Member

@zanieb zanieb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry again about the delay here. I pushed a commit (41bbf8c) with some tweaks and left some notes via review for context.

My focus here was being a bit more verbose and consistent with the error messages, but I made some nit-changes at the same time.

crates/uv/src/commands/tool/run.rs Outdated
return Err(anyhow::anyhow!("Tool command could not be parsed as UTF-8 string. Use `--from` to specify the package name."));
};

if let Some(ref f) = from {
Copy link
Member

@zanieb zanieb Mar 3, 2025

Choose a reason for hiding this comment

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

We try to avoid single letter variables names

crates/uv/src/commands/tool/run.rs Outdated
"It looks you have passed a script instead of a package into `--from`. \n\n{}{} Did you mean to run a tool with `{}`?",
"hint".bold().cyan(),
":".bold(),
format!("{} --from {} {}", invocation_source, PackageName::new(possible_script.to_string_lossy().to_string())?.cyan(), target),
Copy link
Member

@zanieb zanieb Mar 3, 2025

Choose a reason for hiding this comment

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

I think we can probably just the original from string instead of going from String -> Path -> String.

crates/uv/src/commands/tool/run.rs Outdated
return if possible_script.try_exists()? {
Err(anyhow::anyhow!(
"It looks like you are trying to run a script. Did you mean `{}`?",
format!("uv run {}", possible_script.display()).cyan()
Copy link
Member

@zanieb zanieb Mar 3, 2025

Choose a reason for hiding this comment

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

Did you mean to display the absolute path here? user_display is generally preferable, which handles display of paths in the working directory.

crates/uv/tests/it/tool_run.rs Outdated
let context = TestContext::new("3.12").with_filtered_counts();
context.temp_dir.child("script.py").touch()?;

// We treat arguments before the command as uv arguments
Copy link
Member

@zanieb zanieb Mar 3, 2025

Choose a reason for hiding this comment

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

This looks superfluous

crates/uv/src/commands/tool/run.rs Outdated
Comment on lines 118 to 120
"hint".bold().cyan(),
":".bold(),
format!("{} --from {} {}", invocation_source, PackageName::new(possible_script.to_string_lossy().to_string())?.cyan(), target),
Copy link
Member

@zanieb zanieb Mar 3, 2025

Choose a reason for hiding this comment

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

Minor note, rustfmt chokes on macros so you need to format those manually sometimes.

@zanieb
Copy link
Member

zanieb commented Mar 3, 2025

If you think any of the error messages are worse for some reason please let me know so we can align on the best message

@jtfmumm
Copy link
Contributor Author

jtfmumm commented Mar 4, 2025

If you think any of the error messages are worse for some reason please let me know so we can align on the best message

Your changes look good to me.

@zanieb zanieb enabled auto-merge (squash) March 4, 2025 14:14
@zanieb zanieb merged commit c072c9a into main Mar 4, 2025
74 checks passed
@zanieb zanieb deleted the jtfm/uvx-run-hint branch March 4, 2025 14:29
@BrewTestBot BrewTestBot mentioned this pull request Mar 6, 2025
Merged
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Mar 10, 2025
@tmeijn
build(deps): update astral-sh/uv to v0.6.5
b84f60f 
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.6.4` -> `0.6.5` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>astral-sh/uv (astral-sh/uv)</summary>

### [`v0.6.5`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#065)

[Compare Source](astral-sh/uv@0.6.4...0.6.5)

##### Enhancements

-   Allow `--constraints` and `--overrides` in `uvx` ([#&#8203;10207](astral-sh/uv#10207))
-   Allow overrides in `satisfies` check for `uv tool run` ([#&#8203;11994](astral-sh/uv#11994))
-   Allow users to set `package = true` on `tool.uv.sources` ([#&#8203;12014](astral-sh/uv#12014))
-   Add support for Windows legacy scripts via `uv run` ([#&#8203;11888](astral-sh/uv#11888))
-   Return error when running uvx with a `.py` script ([#&#8203;11623](astral-sh/uv#11623))
-   Warn user on use of `uvx run` ([#&#8203;11992](astral-sh/uv#11992))

##### Configuration

-   Add `NO_BUILD` and `NO_BUILD_PACKAGE` environment variables ([#&#8203;11968](astral-sh/uv#11968))

##### Performance

-   Allow overrides in all satisfies checks ([#&#8203;11995](astral-sh/uv#11995))
-   Respect markers on constraints when validating current environment ([#&#8203;11976](astral-sh/uv#11976))

##### Bug fixes

-   Compare major-minor specifiers when filtering interpreters ([#&#8203;11952](astral-sh/uv#11952))
-   Fix system site packages detection default ([#&#8203;11956](astral-sh/uv#11956))
-   Invalidate lockfile when empty dependency groups are added or removed ([#&#8203;12010](astral-sh/uv#12010))
-   Remove prepended sys.path ([#&#8203;11954](astral-sh/uv#11954))
-   Fix PyPy Python version label ([#&#8203;11965](astral-sh/uv#11965))
-   Fix error message suggesting `--user` instead of `--username` ([#&#8203;11947](astral-sh/uv#11947))

##### Preview

-   Move the uv build backend into a separate, minimal `uv_build` package ([#&#8203;11446](astral-sh/uv#11446))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xODguMyIsInVwZGF0ZWRJblZlciI6IjM5LjE4OC4zIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zanieb zanieb zanieb left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Hint use of uv run if uvx is given a script path

3 participants

@jtfmumm @zanieb