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

Follow symbolic links in installed packages #2806

Closed
wants to merge 2 commits into from
Closed

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Apr 3, 2024

Closes #2798

Ok(file_type) => file_type.is_dir().then_some(Ok(entry.path())),
Ok(file_type) => file_type.is_dir().then_some(
// We follow symbolic links to deduplicate linked packages e.g. in `lib` / `lib64`
entry.path().canonicalize(),
Copy link
Member

Choose a reason for hiding this comment

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

What about deduplicating in venv.site_packages()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like it would be faster, yeah. I guess both might make sense. Do you see a downside to this approach?

Copy link
Member

Choose a reason for hiding this comment

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

I can think of contrived cases where this would be wrong. Imagine, e.g., that you symlink anyio-4.0.0.dist-info to anyio-3.7.0.dist-info. I don't know why this would happen. But then we'd resolve them to the same package, which would be wrong. Or imagine you have a symlink to a dist-info directory, like foo symlinks to anyio-4.0.0.dist-info. Then it would appear to be installed twice, when it's not. I don't know, these are weird, but following symlinks on the site-packages directory seems a bit more true to the spirit of what's happening. (I don't really understand why they're symlinked in the first place though.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Then it would appear to be installed twice, when it's not

It wouldn't appear twice in this cause because they'd resolve to the same directory and be de-duplicated?

you symlink anyio-4.0.0.dist-info to anyio-3.7.0.dist-info... then we'd resolve them to the same package

True, but insane haha. I'm not sure if we should account for that.

I'm okay with a more limited implementation, but I'm worried not resolving links here could cause problems in the future too.

@Daverball
Copy link

@zanieb Unfortunately that doesn't appear to have fixed the problem:

> ~/.cargo/bin/uv --version
uv 0.1.28 (192b214dc 2024-04-03)
> ~/.cargo/bin/uv pip install pip
Resolved 1 package in 1ms
warning: Failed to uninstall package at venv/lib/python3.10/site-packages/pip-23.0.1.dist-info due to missing RECORD file. Installation may result in an incomplete environment.
Installed 1 package in 15ms
 - pip==23.0.1
 - pip==23.0.1
 + pip==23.0.1

@zanieb
Copy link
Member Author

zanieb commented Apr 3, 2024

Gahh... that's why we write tests. Thanks for trying it! Will investigate further.

@charliermarsh
Copy link
Member

Superseded by #3002.

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 pip install and uv pip sync both perform each uninstall twice causing the second one to fail
3 participants