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 2 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
19 changes: 14 additions & 5 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 @@ -164,13 +165,21 @@ def download(conanfile, url, filename, verify=True, retry=None, retry_wait=None,
download_cache = config["tools.files.download:download_cache"] if checksum else None

def _download_file(file_url):
# The download cache is only used if a checksum is provided, otherwise, a normal download
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)
if file_url.startswith('file:'):
filepath = _path_from_file_uri(file_url)
shutil.copyfile(filepath, filename)
Copy link
Contributor Author

@jcar87 jcar87 Jul 4, 2022

Choose a reason for hiding this comment

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

I still wonder if this is the best place to do it - we'd have to ignore retry, retry_wait, auth and headers, but still have to check the checksums and overwrite! (working on it!)

The best alternative would be: conans/client/downloaders/file_downloader.py

else:
# The download cache is only used if a checksum is provided, otherwise, a normal download
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)
out.writeln("")

def _path_from_file_uri(uri):
path = urllib.parse.urlparse(uri).path
return urllib.request.url2pathname(path)

if not isinstance(url, (list, tuple)):
_download_file(url)
else: # We were provided several URLs to try
Expand Down
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
27 changes: 27 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,32 @@ 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='this is some content')
file_url = f"file:///{file_location}"

dest = os.path.join(temp_folder(), "file.txt")
download(conanfile, file_url, dest)
content = load(dest)
assert "this is some content" == content

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