Skip to content

fix(cache): separate package cache by platform#2165

Open
Sahitya0805 wants to merge 1 commit intoconda:mainfrom
Sahitya0805:fix/cache-platform-separation
Open

fix(cache): separate package cache by platform#2165
Sahitya0805 wants to merge 1 commit intoconda:mainfrom
Sahitya0805:fix/cache-platform-separation

Conversation

@Sahitya0805
Copy link

@Sahitya0805 Sahitya0805 commented Mar 5, 2026

Description

Previously cache keys did not include platform, causing packages from different platforms to overwrite each other.

This PR stores cached packages under cache/pkgs/<platform>/<package>.

Fixes #1014

How Has This Been Tested?

  • Added test_cache_key_display in cache_key.rs to verify that CacheKey correctly prepends the platform folder.
  • Ran cargo test -p rattler_cache and cargo check locally.

AI Disclosure

  • This PR contains AI-generated content.
    • I have tested any AI-generated content in my PR.
    • I take responsibility for any AI-generated content in my PR.
      Tools: Custom AI Assistant

Checklist:

  • I have performed a self-review of my own code
  • I have added sufficient tests to cover my changes.

@Sahitya0805 Sahitya0805 force-pushed the fix/cache-platform-separation branch from 0dcafca to 678db83 Compare March 5, 2026 01:49
@baszalmstra
Copy link
Collaborator

This doesnt integrate the functionality anywhere and when you do add that its important to think about a migration strategy for people with existing caches.

@ritankarsaha
Copy link
Contributor

This doesnt integrate the functionality anywhere and when you do add that its important to think about a migration strategy for people with existing caches.

Ahh I see the PR here, but I think you have left a comment here mentioning that #2165 is incomplete and doesn't fix the issue, also both addresses different issue tags. Can you look into #2167 once to see if everything is addresses properly over there ?

@Sahitya0805 Sahitya0805 marked this pull request as draft March 5, 2026 11:03
@Sahitya0805 Sahitya0805 force-pushed the fix/cache-platform-separation branch 2 times, most recently from 76d2ac1 to bc7fb49 Compare March 5, 2026 13:42
let mut cache_entry = entry.lock().await;
let cache_path = self.path.join(cache_key.to_string());

let new_path = cache_key.cache_path(&self.path);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This commit says remove duplication but this actually adds a lot of duplication..

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing that out!

You're right the cache path resolution logic is currently duplicated. I'll refactor it by introducing a helper function to resolve the cache path (handling both the new platform aware layout and the legacy layout) and update the PR accordingly.

@baszalmstra
Copy link
Collaborator

Are you still working on this? There is another PR in #2167 trying to solve the same issue. If you are not working on this PR it might be nice for the author of that PR to continue?

@Sahitya0805
Copy link
Author

Yes, I'm still working on it. I'll address the duplication issue and push an update shortly.
If PR #2167 already covers everything needed, I'm also happy to close this PR.

@Sahitya0805 Sahitya0805 force-pushed the fix/cache-platform-separation branch 3 times, most recently from ef41d2a to 1b96c2d Compare March 11, 2026 16:23
@Sahitya0805 Sahitya0805 force-pushed the fix/cache-platform-separation branch from 1b96c2d to 69885c9 Compare March 11, 2026 16:26
@Sahitya0805
Copy link
Author

Thanks for the review!

I refactored the cache path resolution logic into a helper function to remove duplication and updated the call sites accordingly. I also removed the redundant assignments and verified that all tests pass locally, including test_cross_platform_package_cache.

Please let me know if any further changes are needed.

@Sahitya0805 Sahitya0805 marked this pull request as ready for review March 11, 2026 17:05
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.

Cache overwritten by package from different platform.

3 participants