Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions conan/api/subapi/cache.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import json
import os
import shutil
import stat
import tarfile
import tempfile

Expand Down Expand Up @@ -396,6 +397,11 @@ def restore(self, path) -> PackagesList:
fileobj = the_tar.extractfile("pkglist.json")
pkglist = fileobj.read()
the_tar.extraction_filter = (lambda member, _: member) # fully_trusted (Py 3.14)
for member in the_tar.getmembers():
# Extraction opens the destination for writing, read-only files would fail
dest = os.path.join(cache_folder, member.name)
if member.isfile() and os.path.isfile(dest) and not os.access(dest, os.W_OK):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The change is only applied to the files. Could there be a directory with write-only permissions that ends up causing a similar error?

os.chmod(dest, os.stat(dest).st_mode | stat.S_IWRITE)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It doesn't end up with the same permissions it had before. Is this expected?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Exactly, this seems to be forcing some permissions that it didn't have before?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The chmod is temporary and only makes the existing destination writable so extractall() can open it. The tarfile then applies the mode stored in the archive member and file ends with the same permissions it had when saved.

I will improve the tests to reflect this, thanks for pointing it out!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not removing them instead?
Maybe removing existing folders is simpler?

There is still something that concerns me a bit about this, I agree that the current approach of just unzipping on top of the current cache might be not great, maybe it is necessary to think the process of restoring a bit better, beyond a local change of permissions?

the_tar.extractall(path=cache_folder)
the_tar.close()

Expand Down Expand Up @@ -434,7 +440,7 @@ def restore(self, path) -> PackagesList:
if db_pkg_folder != unzipped_pkg_folder:
# If a previous package exists, like a previous restore, then remove it
if os.path.exists(pkg_layout.package()):
shutil.rmtree(pkg_layout.package())
rmdir(pkg_layout.package())
shutil.move(os.path.join(cache_folder, unzipped_pkg_folder),
pkg_layout.package())
pref_bundle["package_folder"] = db_pkg_folder
Expand All @@ -447,7 +453,7 @@ def restore(self, path) -> PackagesList:
if db_metadata_folder != unzipped_metadata_folder:
# We need to put the package in the final location in the cache
if os.path.exists(pkg_layout.metadata()):
shutil.rmtree(pkg_layout.metadata())
rmdir(pkg_layout.metadata())
shutil.move(os.path.join(cache_folder, unzipped_metadata_folder),
pkg_layout.metadata())
pref_bundle["metadata_folder"] = db_metadata_folder
Expand Down
56 changes: 56 additions & 0 deletions test/integration/command/cache/test_cache_save_restore.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import shutil
import sys
import tarfile
import textwrap
import time

import pytest
Expand Down Expand Up @@ -68,6 +69,61 @@ def test_cache_save_restore_with_package_file():
assert tree2 == tree


_READ_ONLY_CONANFILE = textwrap.dedent("""
import os, stat
from conan import ConanFile
from conan.tools.files import save

class Pkg(ConanFile):
name = "pkg"
version = "1.0"
def package(self):
f = os.path.join(self.package_folder, "bin", "readonly.txt")
save(self, f, "content!!")
os.chmod(f, stat.S_IREAD)
""")


def _check_restored_read_only(client):
client.run("list *:*#*")
assert "pkg/1.0" in client.out
ref_layout = client.get_latest_ref_layout(RecipeReference.loads("pkg/1.0"))
pkg_layout = client.get_latest_pkg_layout(PkgReference(ref_layout.reference,
NO_SETTINGS_PACKAGE_ID))
assert load(os.path.join(pkg_layout.package(), "bin", "readonly.txt")) == "content!!"


def test_cache_restore_read_only_files_in_place():
""" restoring over a cache that already contains those same read-only files, the extraction
happens in place, over the existing read-only files
https://github.com/conan-io/conan/issues/20241
"""
c = TestClient()
c.save({"conanfile.py": _READ_ONLY_CONANFILE})
c.run("create .")
c.run("cache save *:*")
c.run("cache restore conan_cache_save.tgz")
_check_restored_read_only(c)


def test_cache_save_restore_read_only_files():
""" restoring in a different cache, the package folder is relocated, so a previously
restored package folder with read-only files has to be removed
https://github.com/conan-io/conan/issues/20241
"""
c = TestClient()
c.save({"conanfile.py": _READ_ONLY_CONANFILE})
c.run("create .")
c.run("cache save *:*")
cache_path = os.path.join(c.current_folder, "conan_cache_save.tgz")

c2 = TestClient()
c2.run(f'cache restore "{cache_path}"')
# The restored files are read-only, restoring again over them must still work
c2.run(f'cache restore "{cache_path}"')
_check_restored_read_only(c2)


def test_cache_save_downloaded_restore():
""" what happens if we save packages downloaded from server, not
created
Expand Down
Loading