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.

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

Fix link-mode=clone on linux #7620

Merged
merged 2 commits into from
Sep 22, 2024
Merged

Fix link-mode=clone on linux #7620

merged 2 commits into from
Sep 22, 2024

Conversation

aroig
Copy link
Contributor

@aroig aroig commented Sep 22, 2024 •
edited
Loading

  • Do not attempt to reflink directories on linux
  • Refactor clone_recursive

Summary

On linux, reflink does not work on a directory. Currently, we first attempt to reflink directory, and only if it fails with AlreadyExists we attempt to reflink recursively.

This has the effect that, on linux, uv pip install --link-mode=clone would always fall back to copy.

We resolve this by only attempting to reflink directories on macos. In the process, we refactored clone_recursive in an attempt to make it easier to reason about its logic.

Test Plan

I tested that after this change, uv pip install --link-mode=clone numpy would behave as expected in the following cases:

  • linux, btrfs filesystem, venv on the same filesystem as cache (correctly reflinked)
  • linux, btrfs filesystem, venv on a different filesystem than cache (fallback to copy)

I have not tested it on macos or windows, as I currently don't have access to any macos or windows machines, unfortunately.

@aroig
Do not attempt to reflink directories on linux
e078a7b 
FICLONE would return EISDIR if the source is a directory. We resolve it
by extending the cases in which we fall back to recursing into the
directory.
@aroig
Copy link
Contributor Author

aroig commented Sep 22, 2024

I pushed a couple of extra commits to make the linter happy, and close a gap for a race condition I missed initially, and caused a CI failure on macos.

This is ready for review.

@charliermarsh
Copy link
Member

Oh smart, thank you. Will review. I’ll admit I’ve never actually tested this on btrfs.

@charliermarsh charliermarsh self-assigned this Sep 22, 2024
@charliermarsh charliermarsh added the bug Something isn't working label Sep 22, 2024
charliermarsh
charliermarsh reviewed Sep 22, 2024
View reviewed changes
crates/install-wheel-rs/src/linker.rs Outdated
clone_recursive(site_packages, wheel, locks, &entry?, attempt)?;
}
return Ok(());
}
Copy link
Member

Choose a reason for hiding this comment

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

Why would it not be sufficient to extend this to Linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. This is the first commit in the PR, actually.

I then tried to decouple a bit this code for the next person who needs to touch it. I found it a bit difficult to reason about because it was handling 2 separate fallback logic, plus the windows and linux special case in clone_recursive.

I do think this makes it easier to follow, but I understand this is somewhat subjective. If you prefer, I can stick with the minimal change that fixes the bug.

Copy link
Member

Choose a reason for hiding this comment

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

While I definitely appreciate the intent, I would prefer to use the minimal change, since this function is so critical and making large changes to it requires extensive testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I updated the branch with a minimal change to fix the bug.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. This I can easily and confidently merge.

@charliermarsh charliermarsh merged commit 542afe7 into astral-sh:main Sep 22, 2024
58 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Sep 24, 2024
Merged
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Oct 7, 2024
@tmeijn
build(deps): update astral-sh/uv to v0.4.18
f9ced3a 
This MR contains the following updates:

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

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

[Compare Source](astral-sh/uv@0.4.17...0.4.18)

##### Enhancements

-   Allow multiple source entries for each package in `tool.uv.sources` ([#&#8203;7745](astral-sh/uv#7745))
-   Add `.gitignore` file to `uv build` output directory ([#&#8203;7835](astral-sh/uv#7835))
-   Disable jemalloc on FreeBSD ([#&#8203;7780](astral-sh/uv#7780))
-   Respect `PAGER` env var when paging in `uv help` command ([#&#8203;5511](astral-sh/uv#5511))
-   Support `uv run -m foo` to run a module ([#&#8203;7754](astral-sh/uv#7754))
-   Use a top-level output directory for `uv build` in workspaces ([#&#8203;7813](astral-sh/uv#7813))
-   Update `uv init --package` command to match project name ([#&#8203;7670](astral-sh/uv#7670))
-   Add a custom suggestion for `uv add dotenv` ([#&#8203;7799](astral-sh/uv#7799))
-   Add detailed errors for `tool.uv.sources` deserialization failures ([#&#8203;7823](astral-sh/uv#7823))
-   Improve error message copy for failed builds ([#&#8203;7849](astral-sh/uv#7849))
-   Use `serde-untagged` to improve some untagged enum error messages ([#&#8203;7822](astral-sh/uv#7822))
-   Use build failure hints for `dotenv` errors, rather than in `uv add` ([#&#8203;7825](astral-sh/uv#7825))

##### Configuration

-   Add `UV_NO_SYNC` environment variable ([#&#8203;7752](astral-sh/uv#7752))

##### Bug fixes

-   Accept `git+` prefix in `tool.uv.sources` ([#&#8203;7847](astral-sh/uv#7847))
-   Allow spaces in path requirements ([#&#8203;7767](astral-sh/uv#7767))
-   Avoid reusing cached downloaded binaries with `--no-binary` ([#&#8203;7772](astral-sh/uv#7772))
-   Correctly trims values during wheel WHEEL file parsing ([#&#8203;7770](astral-sh/uv#7770))
-   Fix `uv tree --invert` for platform dependencies ([#&#8203;7808](astral-sh/uv#7808))
-   Fix encoding mismatch between python child process and uv ([#&#8203;7757](astral-sh/uv#7757))
-   Reject self-dependencies in `uv add` ([#&#8203;7766](astral-sh/uv#7766))
-   Respect `tool.uv.environments` for legacy virtual workspace roots ([#&#8203;7824](astral-sh/uv#7824))
-   Retain empty extras on workspace members ([#&#8203;7762](astral-sh/uv#7762))
-   Use file stem when parsing cached wheel names ([#&#8203;7773](astral-sh/uv#7773))

##### Rust API

-   Make `FlatDistributions` public ([#&#8203;7833](astral-sh/uv#7833))

##### Documentation

-   Fix table of contents sizing ([#&#8203;7751](astral-sh/uv#7751))
-   GitLab Integration documentation ([#&#8203;6857](astral-sh/uv#6857))
-   Update documentation to setup-uv@v3 ([#&#8203;7807](astral-sh/uv#7807))
-   Use `uv publish` instead of twine in docs ([#&#8203;7837](astral-sh/uv#7837))
-   Fix typo in `projects.md` ([#&#8203;7784](astral-sh/uv#7784))

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

[Compare Source](astral-sh/uv@0.4.16...0.4.17)

##### Enhancements

-   Add `uv build --all` to build all packages in a workspace ([#&#8203;7724](astral-sh/uv#7724))
-   Add support for `uv init --script` ([#&#8203;7565](astral-sh/uv#7565))
-   Add support for upgrading build environment for installed tools (`uv tool upgrade --python`) ([#&#8203;7605](astral-sh/uv#7605))
-   Initialize a Git repository in `uv init` ([#&#8203;5476](astral-sh/uv#5476))
-   Respect `--quiet` flag in `uv build` ([#&#8203;7674](astral-sh/uv#7674))
-   Add context message before listing available tools in `uvx` ([#&#8203;7641](astral-sh/uv#7641))

##### Bug fixes

-   Don't create Python bytecode files during interpreter discovery ([#&#8203;7707](astral-sh/uv#7707))
-   Escape glob patterns in workspace member discovery ([#&#8203;7709](astral-sh/uv#7709))
-   Avoid prefetching source distributions with unbounded lower-bound ranges ([#&#8203;7683](astral-sh/uv#7683))

##### Documentation

-   Add `uv build` and `uv publish` to features overview ([#&#8203;7716](astral-sh/uv#7716))
-   Add documentation on cache versioning ([#&#8203;7693](astral-sh/uv#7693))
-   Spell out the names of the Docker images for easier copy-paste ([#&#8203;7706](astral-sh/uv#7706))
-   Document uv-with-Jupyter workflows ([#&#8203;7625](astral-sh/uv#7625))
-   Note that `uv lock --upgrade-package` retains locked versions ([#&#8203;7694](astral-sh/uv#7694))

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

[Compare Source](astral-sh/uv@0.4.15...0.4.16)

##### Enhancements

-   Add `uv publish` ([#&#8203;7475](astral-sh/uv#7475))
-   Add a `--project` argument to run a command from a project directory ([#&#8203;7603](astral-sh/uv#7603))
-   Display Python implementation when creating environments ([#&#8203;7652](astral-sh/uv#7652))
-   Implement trusted publishing for `uv publish` ([#&#8203;7548](astral-sh/uv#7548))
-   Respect lockfile preferences for `--with` requirements ([#&#8203;7627](astral-sh/uv#7627))
-   Unhide the `--directory` option ([#&#8203;7653](astral-sh/uv#7653))
-   Allow requesting free-threaded Python interpreters ([#&#8203;7431](astral-sh/uv#7431))
-   Show a dedicated PubGrub hint for `--unsafe-best-match` ([#&#8203;7645](astral-sh/uv#7645))
-   Add resolver error checking for conflicting distributions ([#&#8203;7595](astral-sh/uv#7595))

##### Bug fixes

-   Avoid adding double-newlines for CRLF ([#&#8203;7640](astral-sh/uv#7640))
-   Avoid retaining forks when `requires-python` range changes ([#&#8203;7624](astral-sh/uv#7624))
-   Determine if pre-release Python downloads should be allowed using the version specifiers ([#&#8203;7638](astral-sh/uv#7638))
-   Fix `link-mode=clone` for directories on Linux ([#&#8203;7620](astral-sh/uv#7620))
-   Improve Python executable name discovery when using alternative implementations ([#&#8203;7649](astral-sh/uv#7649))
-   Require opt-in to use alternative Python implementations ([#&#8203;7650](astral-sh/uv#7650))
-   Use the first pre-release discovered when only pre-release Python versions are available ([#&#8203;7666](astral-sh/uv#7666))

##### Documentation

-   Document environment variable that disables printing of virtual environment name in prompt ([#&#8203;7648](astral-sh/uv#7648))
-   Remove double whitespaces from the code ([#&#8203;7623](astral-sh/uv#7623))
-   Use anchorlinks rather than permalinks ([#&#8203;7626](astral-sh/uv#7626))

##### Preview features

-   Add build backend scaffolding ([#&#8203;7662](astral-sh/uv#7662))

</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
Reviewers

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees

@charliermarsh charliermarsh

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aroig @charliermarsh