Skip to content
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.

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

Enforce correctness of self-dependencies #9705

Merged
merged 3 commits into from
Dec 8, 2024
Merged

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Dec 7, 2024

Summary

As far as I can tell, this was added in #319, but it seems incorrect to ignore these.

Closes #9693.

@charliermarsh charliermarsh added the bug Something isn't working label Dec 7, 2024
@zanieb
Copy link
Member

zanieb commented Dec 7, 2024

Hm I'm a little worried about the implications of this. Will it break people with dynamic versions?

@charliermarsh
Copy link
Member Author

Why would it break people with dynamic versions?

@charliermarsh
Copy link
Member Author

(This only affects packages that declare dependencies on themselves, which is itself almost always a mistake.)

@zanieb
Copy link
Member

zanieb commented Dec 7, 2024

I'm imagining you have a self-dependency with a version constraint that.. works in production but fails locally due some weird dynamic development version scheme? Perhaps I'm overthinking it. Dynamic versions stood out to me as a case where there are weird local versions, but I don't think that's the only case where there could be a mismatch.

More broadly, what happens with a self-dependency in production i.e. published to PyPI?

@charliermarsh
Copy link
Member Author

More broadly, what happens with a self-dependency in production i.e. published to PyPI?

Can you expand on this? If you install the package from PyPI, and it declares a dependency on itself, they will just resolve to the same package. The same is true if you clone the package and install it locally. Both would work fine.

The case we would fail is: you declare a self-dependency, but at a version that is incompatible with the current package version. I think that should fail! Check out the linked issue where we silently succeed with a broken resolution.

(I don’t know that there’s even a real, valid use-case for a self-dependency.)

@zanieb
Copy link
Member

zanieb commented Dec 7, 2024

I did read the linked issue :) just trying to understand the problem more broadly since it turns something that's currently allowed into a firm error.

The case we would fail is: you declare a self-dependency, but at a version that is incompatible with the current package version. I think that should fail!

I guess the clarifying question is: would this fail at publish or install time to / from PyPI? (entirely separately from uv)

@charliermarsh
Copy link
Member Author

By “publish time”, do you mean, if you ran uv lock locally prior to publishing?

@zanieb
Copy link
Member

zanieb commented Dec 7, 2024

I mean.. if you weren't using uv at all, i.e., some lower-level tools like python -m build and twine.

@charliermarsh
Copy link
Member Author

Hmm, none of those tools have to resolve dependencies though. There’s no dependency validation. They just have to construct the metadata. They don’t have to solve the graph.

@zanieb
Copy link
Member

zanieb commented Dec 7, 2024

Yeah so then when you install with pip later what would happen? Perhaps there would be a failure then?

@charliermarsh
Copy link
Member Author

Yeah it would fail then. Though I think that’s the same as if, e.g., the package declared dependencies on both flask==3.0.0 and flask==2.0.0? It would fail on install, but not on publish.

@zanieb
Copy link
Member

zanieb commented Dec 7, 2024

Okay, thanks for going through it with me. I'm not quite sure where this change could cause problems 👍

(I don’t know that there’s even a real, valid use-case for a self-dependency.)

I guess it'd let you declare a default extra that.. you can't turn off? 😬

@charliermarsh charliermarsh force-pushed the charlie/self-deps branch 3 times, most recently from a4d880c to eb55dff Compare December 7, 2024 18:03
@charliermarsh charliermarsh marked this pull request as ready for review December 7, 2024 18:03

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because your project depends on your project and your project requires your project, we can conclude that your projects's requirements are unsatisfiable.
Copy link
Member Author

Choose a reason for hiding this comment

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

Gah, need to fix this error message.

Copy link
Member

Choose a reason for hiding this comment

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

I thought we already had a hint for self dependencies 🤔

Copy link
Member

@zanieb zanieb Dec 7, 2024

Choose a reason for hiding this comment

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

I was thinking of #7505 (which may help guide the messaging here?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added hints!

@charliermarsh charliermarsh force-pushed the charlie/self-deps branch 2 times, most recently from 5406bc2 to 7fc162b Compare December 7, 2024 18:26
@@ -4450,6 +4450,9 @@ wheels = [
name = "tensorflow-macos"
version = "2.15.1"
source = { registry = "https://pypi.org/simple" }
dependencies = [
{ name = "tensorflow-macos", marker = "platform_machine == 'arm64' and platform_system == 'Darwin'" },
]
Copy link
Member Author

Choose a reason for hiding this comment

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

We could choose to omit these? This is a useless self-dependency. I retained them but we could omit them when converting from PubGrub resolution to our own graph.


----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because your project depends on itself at an incompatible version, we can conclude that your project's requirements are unsatisfiable.
Copy link
Member Author

Choose a reason for hiding this comment

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

Kind of weak.

Copy link
Member

Choose a reason for hiding this comment

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

I'd include the specifier and the version maybe?

Suggested change
╰─▶ Because your project depends on itself at an incompatible version, we can conclude that your project's requirements are unsatisfiable.
╰─▶ Because your project depends on itself (project==0.2.0) at an incompatible version (currently 0.1.0), we can conclude that your project's requirements are unsatisfiable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually don't think I can get the version here right now :/


----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because project[foo] depends on your project and your project requires project[foo], we can conclude that your project's requirements are unsatisfiable.
Copy link
Member Author

Choose a reason for hiding this comment

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

Kind of weak.

@charliermarsh charliermarsh force-pushed the charlie/self-deps branch 2 times, most recently from 23cc863 to decf6cd Compare December 7, 2024 19:16
@charliermarsh charliermarsh force-pushed the charlie/self-deps branch 4 times, most recently from 2936a4f to 3d10a30 Compare December 8, 2024 04:00
@charliermarsh charliermarsh enabled auto-merge (squash) December 8, 2024 04:00
@charliermarsh charliermarsh merged commit 1b4bd8d into main Dec 8, 2024
64 checks passed
@charliermarsh charliermarsh deleted the charlie/self-deps branch December 8, 2024 04:17
@notatallshaw-gts
Copy link

Just want to check this doesn't break the real use case of using extras to depend on other extras of your own project? e.g.

[project]
name = "spam-eggs"
version = "2020.0.0"
requires-python = ">=3.8"

[project.optional-dependencies]
tests = ["pytest"]
dev = ["pdb", "memray"]
all = [
  "spam-eggs[tests]",
  "spam-eggs[dev]",
]

@charliermarsh
Copy link
Member Author

No, that remains totally fine.

@charliermarsh
Copy link
Member Author

charliermarsh commented Dec 9, 2024

It would error if you did:

[project]
name = "spam-eggs"
version = "2020.0.0"
requires-python = ">=3.8"

[project.optional-dependencies]
tests = ["pytest"]
all = [
  # Incompatible version!
  "spam-eggs[tests]==2021.0.0",
]

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Dec 12, 2024
This MR contains the following updates:

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

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.5.8`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#058)

[Compare Source](astral-sh/uv@0.5.7...0.5.8)

**This release does not include the `powerpc64le-unknown-linux-musl` target due to a build issue. See [#&#8203;9793](astral-sh/uv#9793) for details. If this change affects you, please file an issue with your use-case.**

##### Enhancements

-   Omit empty resolution markers in lockfile  ([#&#8203;9738](astral-sh/uv#9738))
-   Add `--install-dir` to to `uv python install` and `uninstall` commands ([#&#8203;7920](astral-sh/uv#7920))
-   Add `--show-urls` and `--only-downloads` to `uv python list` ([#&#8203;8062](astral-sh/uv#8062))
-   Add `uv python list --all-arches` ([#&#8203;9782](astral-sh/uv#9782))
-   Add `uv run --gui-script` flag for running Python scripts with `pythonw.exe` ([#&#8203;9152](astral-sh/uv#9152))
-   Allow `--gui-script` on Unix ([#&#8203;9787](astral-sh/uv#9787))
-   Allow download of Python distribution variants optimized for newer x86\_64 microarchitectures ([#&#8203;9781](astral-sh/uv#9781))
-   Allow execution of `pyw` files on Unix ([#&#8203;9759](astral-sh/uv#9759))
-   Allow users to specify URLs in `project.dependencies` and `tool.uv.sources` ([#&#8203;9718](astral-sh/uv#9718))
-   Encode mutually-incompatible pairs of markers ([#&#8203;9444](astral-sh/uv#9444))
-   Improve the error message when a Python install request is not valid ([#&#8203;9783](astral-sh/uv#9783))
-   Preserve directory-level standalone build symlinks ([#&#8203;9723](astral-sh/uv#9723))
-   Add support for `uv publish --index <name>` ([#&#8203;9694](astral-sh/uv#9694))
-   Reframe `--locked` and `--frozen` as `--check` operations for `uv lock` ([#&#8203;9662](astral-sh/uv#9662))
-   Rename Python install scratch directory from `.cache` -> `.temp` ([#&#8203;9756](astral-sh/uv#9756))
-   Enable `uv tool uninstall uv` on Windows ([#&#8203;8963](astral-sh/uv#8963))
-   Improve self-dependency hint to make shadowing clear ([#&#8203;9716](astral-sh/uv#9716))
-   Refactor unavailable metadata to shrink the resolver ([#&#8203;9769](astral-sh/uv#9769))
-   Show 'depends on itself' for proxy packages ([#&#8203;9717](astral-sh/uv#9717))
-   Show a dedicated error for missing subdirectories ([#&#8203;9761](astral-sh/uv#9761))
-   Show a dedicated hint for missing `git+` prefixes ([#&#8203;9789](astral-sh/uv#9789))

##### Performance

-   Eagerly error when parsing `pyproject.toml` requirements ([#&#8203;9704](astral-sh/uv#9704))
-   Use copy-on-write when normalizing paths ([#&#8203;9710](astral-sh/uv#9710))

##### Bug fixes

-   Avoid enforcing non-conflicts in `uv export` ([#&#8203;9751](astral-sh/uv#9751))
-   Don't drop comments between items in TOML tables ([#&#8203;9784](astral-sh/uv#9784))
-   Don't fail with `--no-build` when static metadata is available ([#&#8203;9785](astral-sh/uv#9785))
-   Don't filter non-patch registry version ([#&#8203;9736](astral-sh/uv#9736))
-   Don't read metadata from stale `.egg-info` files ([#&#8203;9760](astral-sh/uv#9760))
-   Enforce correctness of self-dependencies ([#&#8203;9705](astral-sh/uv#9705))
-   Fix projects's typo in resolver error messages ([#&#8203;9708](astral-sh/uv#9708))
-   Ignore `.` prefixed directories during managed Python installation discovery ([#&#8203;9786](astral-sh/uv#9786))
-   Improve handling of invalid virtual environments during interpreter discovery ([#&#8203;8086](astral-sh/uv#8086))
-   Normalize relative paths when `--project` is specified ([#&#8203;9709](astral-sh/uv#9709))
-   Respect self-constraints on recursive extras ([#&#8203;9714](astral-sh/uv#9714))
-   Respect user settings for tracing coloring ([#&#8203;9733](astral-sh/uv#9733))
-   Retry on tar extraction errors ([#&#8203;9753](astral-sh/uv#9753))
-   Add conflict markers to the lock file ([#&#8203;9370](astral-sh/uv#9370))
-   De-duplicate resolution markers ([#&#8203;9780](astral-sh/uv#9780))
-   Avoid 403 error hint for PyTorch URLs ([#&#8203;9750](astral-sh/uv#9750))
-   Avoid treating non-existent `--find-links` as relative URLs ([#&#8203;9720](astral-sh/uv#9720))
-   Omit Windows Store `python3.13.exe` et al ([#&#8203;9679](astral-sh/uv#9679))
-   Replace executables with broken symlinks during `uv python install` ([#&#8203;9706](astral-sh/uv#9706))

##### Documentation

-   Fix build failure links ([#&#8203;9740](astral-sh/uv#9740))

</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:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uv incorrectly (?) installs dbt-clickhouse
3 participants