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 conan cache restore when restoring the same cache multiple times #15950

Merged
merged 3 commits into from
Mar 27, 2024

Conversation

obnyis
Copy link
Contributor

@obnyis obnyis commented Mar 26, 2024

Changelog: Bugfix: Make conan cache restore work correctly when restoring over a package already in the local cache.
Docs: Omit

Close #15949

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

conan/api/subapi/cache.py Outdated Show resolved Hide resolved
@obnyis obnyis force-pushed the fix-cache-multiple-restore branch from a47bca2 to 09c8d9b Compare March 26, 2024 11:41
@obnyis obnyis force-pushed the fix-cache-multiple-restore branch from 09c8d9b to 4d7c955 Compare March 26, 2024 11:42
@CLAassistant
Copy link

CLAassistant commented Mar 26, 2024

CLA assistant check
All committers have signed the CLA.

@memsharded memsharded self-assigned this Mar 26, 2024
Copy link
Member

@memsharded memsharded left a 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

@@ -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):
Copy link
Member

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.

Copy link
Member

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

@memsharded
Copy link
Member

I have pushed some minor changes mostly .replace("\\", "/") to make things work in Windows

@memsharded memsharded added this to the 2.3.0 milestone Mar 26, 2024
@obnyis
Copy link
Contributor Author

obnyis commented Mar 26, 2024

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

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

@memsharded memsharded merged commit 98fab79 into conan-io:develop2 Mar 27, 2024
2 checks passed
@memsharded
Copy link
Member

Thanks very much for contributing this. It is merged, it will be in next 2.3 release.

@obnyis obnyis deleted the fix-cache-multiple-restore branch March 27, 2024 09:50
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.

[bug] conan cache restore produces incorrect folders if run multiple times
3 participants