-
Notifications
You must be signed in to change notification settings - Fork 981
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 conan cache restore when restoring the same cache multiple times #15950
Fix conan cache restore when restoring the same cache multiple times #15950
Conversation
a47bca2
to
09c8d9b
Compare
09c8d9b
to
4d7c955
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for your contribution, and thanks very much for the detailed report in the issue @obnyis
I see some tests are failing in Windows conans/test/integration/command_v2/test_cache_save_restore.py::test_cache_save_downloaded_restore
, though I think our CI logs are closed at the moment (some security issues), it reports issues with the Windows paths:
The system cannot find the file specified: 'C:\\J\\t_v2\\4c3f\\PY36\\tmpykqzx4v6conans\\path with spaces\\.conan2\\p/pkgff8e5eaab3983/p' -> 'C:\\J\\t_v2\\4c3f\\PY36\\tmpykqzx4v6conans\\path with spaces\\.conan2\\p\\pkgff8e5eaab3983\\p'
This is something I can have a look if not easy to test in Windows for you
conan/api/subapi/cache.py
Outdated
@@ -180,14 +180,22 @@ def restore(self, path): | |||
pkg_layout = cache.get_or_create_pkg_layout(pref) | |||
pkg_folder = pref_bundle["package_folder"] | |||
out.info(f"Restore: {pref} in {pkg_folder}") | |||
# We need to put the package in the final location in the cache | |||
shutil.move(os.path.join(cache.cache_folder, pkg_folder), pkg_layout.package()) | |||
if pkg_layout.package() != os.path.join(cache.cache_folder, pkg_folder): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very evident what this comparison is trying to protect against or do. I would say that these could match, and still the package should be restored, but it seems in some cases it would be skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I have understood it is an optimization to not have to process folders already in place
I have pushed some minor changes mostly |
Ah sorry about that. I added a guard clause around the directory deletion just before I left for the night, and forgot to consider the implications on windows paths. I'm fine with running windows tests :) just forgot to set up a copy before I pushed this up |
Thanks very much for contributing this. It is merged, it will be in next 2.3 release. |
Changelog: Bugfix: Make
conan cache restore
work correctly when restoring over a package already in the local cache.Docs: Omit
Close #15949
develop
branch, documenting this one.