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

Add ability to reference files in the local filesystem with file:// s… #11569

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions conan/tools/files/files.py
Expand Up @@ -11,6 +11,7 @@
from fnmatch import fnmatch

import six
import urllib

from conan.tools import CONAN_TOOLCHAIN_ARGS_FILE, CONAN_TOOLCHAIN_ARGS_SECTION
from conan.tools.apple.apple import is_apple_os
Expand Down Expand Up @@ -165,10 +166,11 @@ def download(conanfile, url, filename, verify=True, retry=None, retry_wait=None,

def _download_file(file_url):
# The download cache is only used if a checksum is provided, otherwise, a normal download
local_filesystem = True if file_url.startswith("file:") else False
run_downloader(requester=requester, output=out, verify=verify, download_cache=download_cache,
user_download=True, url=file_url,
file_path=filename, retry=retry, retry_wait=retry_wait, overwrite=overwrite,
auth=auth, headers=headers, md5=md5, sha1=sha1, sha256=sha256)
user_download=True, url=file_url, local_filesystem=local_filesystem,
file_path=filename, retry=retry, retry_wait=retry_wait, overwrite=overwrite,
auth=auth, headers=headers, md5=md5, sha1=sha1, sha256=sha256)
out.writeln("")

if not isinstance(url, (list, tuple)):
Expand Down
9 changes: 6 additions & 3 deletions conans/client/downloaders/download.py
@@ -1,11 +1,14 @@
from conans.client.downloaders.cached_file_downloader import CachedFileDownloader
from conans.client.downloaders.file_downloader import FileDownloader
from conans.client.downloaders.local_file_downloader import LocalFileDownloader


def run_downloader(requester, output, verify, retry, retry_wait, download_cache, user_download=False,
**kwargs):
def run_downloader(requester, output, verify, retry, retry_wait, download_cache, local_filesystem,
user_download=False, **kwargs):
downloader = FileDownloader(requester=requester, output=output, verify=verify,
config_retry=retry, config_retry_wait=retry_wait)
if download_cache:
if local_filesystem:
downloader = LocalFileDownloader(output=output)
elif download_cache:
downloader = CachedFileDownloader(download_cache, downloader, user_download=user_download)
return downloader.download(**kwargs)
29 changes: 29 additions & 0 deletions conans/client/downloaders/local_file_downloader.py
@@ -0,0 +1,29 @@
import os
from urllib.parse import urlparse
from urllib.request import url2pathname
from shutil import copyfile

from conans.client.tools.files import check_md5, check_sha1, check_sha256
from conans.errors import ConanException


class LocalFileDownloader(object):

def __init__(self, output):
self._output = output

def download(self, url, file_path, md5=None, sha1=None, sha256=None, **kwargs):

file_origin = self._path_from_file_uri(url)
copyfile(file_origin, file_path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note here: shutil.copyfile() will overwrite the file if it already exists - I've not added any logic to handle that case. From what I can see, in the newer implementation we are defaulting to overwrite=True always, and the overwrite argument is no longer exposed.
See here: https://github.com/conan-io/conan/blob/develop/conan/tools/files/files.py#L151

But I can see the file downloader does interpret this flag:
https://github.com/conan-io/conan/blob/develop/conans/client/downloaders/file_downloader.py#L46-L53

If there are plans to make overwrites configurable (via conf like with the other options), then it's worth implementing now


if md5:
check_md5(file_path, md5)
if sha1:
check_sha1(file_path, sha1)
if sha256:
check_sha256(file_path, sha256)

def _path_from_file_uri(self, uri):
path = urlparse(uri).path
return url2pathname(path)
1 change: 1 addition & 0 deletions conans/requirements_dev.txt
@@ -1,6 +1,7 @@
pytest>=6.1.1, <7.0.0; python_version > '3.0'
pytest>=4.6.11; python_version < '3.0'
pytest-xdist # To launch in N cores with pytest -n
pyfakefs>=4.5.6
Copy link
Member

Choose a reason for hiding this comment

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

I'd say definitely not worth the complexities and problems for using more third parties, for this very specific use case.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn't realize it was requirements_dev.txt. No problem with this one, if it really helps for the test (if there is an easy way to do the test without it, better, of course)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, just used for testing!

I exposes an in-memory filesystem to tests such that python functions that operate on the filesystem read from this instead.

For the purposes of this test, writing a temp file on the actual file system and reading back is also an option. I wanted to a be a little more explicit making sure that paths like file:///C:/path/to/file.txt are resolved properly on Windows - but I think if we used the existing temp_folder() functionality in the test, the paths would be implicitly platform-appropriate anyway, so we would achieve the same testing. Let me know if this is preferred!

Copy link
Member

Choose a reason for hiding this comment

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

I'd say yes, we already do a ton of temp_folder() in our tests suite, and I think this could perfectly work, please give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Still does the job :D

parameterized>=0.6.3
mock>=1.3.0, <1.4.0
WebTest>=2.0.18, <2.1.0
Expand Down
28 changes: 28 additions & 0 deletions conans/test/unittests/tools/files/test_downloads.py
@@ -1,4 +1,5 @@
import os
import platform

import pytest
import requests
Expand Down Expand Up @@ -157,6 +158,33 @@ def test_download_no_retries_errors(self, bottle_server):
assert "Waiting" not in str(conanfile.output)
assert "retry" not in str(conanfile.output)

def test_download_localfile(self, fs):
conanfile = ConanFileMock()
conanfile._conan_requester = requests

file_location = '/path/to/file.txt'
if platform.system() == "Windows":
file_location = "C:" + file_location
fs.create_file(file_location, contents=b'this is some content\n')
file_url = f"file:///{file_location}"
file_md5 = "a0b156435474e688206c68e5c66a3327"

dest = os.path.join(temp_folder(), "file.txt")
download(conanfile, file_url, dest, md5=file_md5)
content = load(dest)
assert "this is some content" == content.rstrip()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert "this is some content" == content.rstrip()
assert "this is some content" == content

The rstrip() is probably unnecessary, and risks hiding a bug somewhere. The file must be identical

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it by changing the md5 that is checked above (as it stands, given the md5 check, this is probably redundant).

Issue was that most command line text editors and the shell itself will append a newline character, so all of my attempts to get the md5 sum of a string will give me the md5 sum of a string ending with the newline character. Given that a few lines above we have control of the exact byte-wise contents of the file, I think it's safe to remove the rstrip()


def test_download_localfile_notfound(self):
conanfile = ConanFileMock()
conanfile._conan_requester = requests

file_url = "file:///path/to/missing/file.txt"
dest = os.path.join(temp_folder(), "file.txt")

with pytest.raises(FileNotFoundError) as exc:
download(conanfile, file_url, dest)

assert "No such file" in str(exc.value)

@pytest.fixture()
def bottle_server_zip():
Expand Down