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

Handle package cache in secondary locations. #856

Merged
merged 3 commits into from
Apr 19, 2021

Conversation

wenjuno
Copy link
Contributor

@wenjuno wenjuno commented Apr 15, 2021

This is the fix for #852. I changed my mind about the solution and I think it's a better and easier one:

  1. If there is extracted cache, use it, otherwise next.
  2. If there is tarball in the first writable cache path, extract it, otherwise next.
  3. Run the full download pipeline.

I have two questions that I think might be related to the decision I'm not aware of but I'm very curious about them:

  1. In py_interface.cpp the execute member function of MTransaction is wrapped in a lambda function. Why not use the member function's address?
  2. The member PackageCacheData::m_valid_cache is std::map. Why not use std::unorder_map instead? I basically follow the style and use std::map for the cache in MultiPackageCache.

Thanks.

@wenjuno
Copy link
Contributor Author

wenjuno commented Apr 16, 2021

Previous failure was because I was using cpp-filesystem 1.4 and it has a bug with exists function such that it returns true for dangling symlink. The bug was fixed in 1.5.4: gulrak/filesystem#107. In the last commit I add a check with is_symlink so if the file is a symlink it will consider it exist no matter whether it's a dangling symlink or not.

@wolfv
Copy link
Member

wolfv commented Apr 16, 2021

Hi @wenjuno thanks so much for this PR! We're currently preparing a release, and we'll review this right after. I hope we can make another release early next week to get this in!

@wolfv
Copy link
Member

wolfv commented Apr 16, 2021

btw. we also just updated to the latest cpp-filesystem (and faced some issues). Hopefully those issues are resolved for now (mostly around long path support on Windows). I am happy to hear that the update also fixed something for you!

@wenjuno
Copy link
Contributor Author

wenjuno commented Apr 16, 2021

Hi @wolfv, thanks. I see you changed the master branch, do you want me to rebase my branch?

To be clear, the cpp-filesystem 1.5 didn't fix something for me, maybe my comment above is a bit confusing. What I mean is I was using 1.4 for local testing and it has the bug so the code worked for me but CI has started using 1.5 around the time so it failed the CI test the first time. I went back and found out the code that relies on the 1.4 behavior (the exists function) and fixed that in my latest commit. The change of behavior also reflects in the master branch CI. You can see the warning message: "WARNING Missing file from package cache: "/home/runner/mambaroot/pkgs/_openmp_mutex-4.5-1_gnu/lib/libgomp.so.1"". After this warning mamba will remove the package cache of _openmp_mutex. Later on mamba will extract the package cache from tarball but the logic in master doesn't check the package cache again so it doesn't remove the package cache again so it works when linking the package files.

Thanks.

@wenjuno
Copy link
Contributor Author

wenjuno commented Apr 16, 2021

There is a fork in the history so I rebased my three commits.

@wolfv wolfv merged commit f1122f4 into mamba-org:master Apr 19, 2021
@wolfv
Copy link
Member

wolfv commented Apr 19, 2021

thanks for fixing this symlink extract bug and improving the situation with multiple package caches! Excellent PR

@adriendelsalle adriendelsalle added this to the 0.10.1 milestone Apr 20, 2021
@wenjuno
Copy link
Contributor Author

wenjuno commented Apr 21, 2021

Thanks, I'm glad I can help. I will close the associated issue.

chrisburr added a commit to chrisburr/cf-scripts that referenced this pull request Apr 21, 2021
@wenjuno wenjuno deleted the fix-pkg-cache branch April 21, 2021 22:34
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