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

fix: consistent clobbering & removal of __pycache__ #437

Merged
merged 10 commits into from
Jan 18, 2024

Conversation

wolfv
Copy link
Contributor

@wolfv wolfv commented Dec 15, 2023

This attempts to fix two issues:

  • remove empty __pycache__ folders for noarch packages (otherwise the package is still "detected" in the site-packages folder).
  • do not attempt to link individual files more than once per installation. This happens when there are "clobbering" files. Currently a random file will win (due to async execution).

However, there is still two remaining issues with clobbering:

  • We should deterministically decide on a "winner". I think it should be by topologically sorting the packages and take the last (top) package as a winner.
  • When removing packages we also need to take care of the (un-)clobbering. This is a bit more tricky because we might not even have the clobbering package in the package cache anymore. However, I think a workaround could be to still link the files into the environment but with the name of the source package appended (e.g. crates2.json would become crates2.json-clobber-from-mypackage. For unclobbering we could just rename the file.

@wolfv
Copy link
Contributor Author

wolfv commented Dec 15, 2023

cc @Wackyator we could also re-use the unlink_package in the Python bindings! :)

Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Nice! I like the simplicity!

crates/rattler/src/install/mod.rs Outdated Show resolved Hide resolved
crates/rattler/Cargo.toml Outdated Show resolved Hide resolved
@Wackyator
Copy link
Contributor

looks awesome, will add this to the bindings :)

Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

I like this a lot @wolfv ! Lets get it merged asap!

crates/rattler/src/install/driver.rs Show resolved Hide resolved
@wolfv wolfv force-pushed the fix-clobber branch 6 times, most recently from e207ae9 to e1de44a Compare January 11, 2024 11:26
@wolfv
Copy link
Contributor Author

wolfv commented Jan 11, 2024

OK, now I just need to write some tests for the unlinking parts and then I think we're done with this one :) ... the easiest would be to resolve some actual packages, install, then unlink them. But I don't think we have a test harness for that yet.

@wolfv
Copy link
Contributor Author

wolfv commented Jan 11, 2024

@baszalmstra when you get around to review this, please make sure to really carefully review the unlink parts because if that goes south, we could delete user data which we definitely would not want!

@wolfv
Copy link
Contributor Author

wolfv commented Jan 16, 2024

@baszalmstra I took care of your comments. Two things we might still want to do:

  • test "deeper nested" clobbers (bla/bli/clobber.txt).
  • add something about clobbers to conda-meta files.

@baszalmstra baszalmstra changed the title Fix clobber & __pycache__ issues fix: consistent clobbering & removal of __pycache__ Jan 18, 2024
@baszalmstra baszalmstra merged commit 85ca4ef into conda:main Jan 18, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants