#6349 Add support for mirror when downloading files #6679
Conversation
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
uilianries
changed the title
Signed-off-by: Uilian Ries <uilianries@gmail.com>
This reverts commit f554ed8.
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
|
@memsharded the #6679 is ready for review! |
Signed-off-by: Uilian Ries <uilianries@gmail.com>
IMO we should allow only one checksum, the same for all the URLs. If you think that's not the case, you will need to convince me
Signed-off-by: Uilian Ries <uilianries@gmail.com>
|
@jgsogo please, take a look again. Now the same checksum should work for all urls. Also, I mocked the Requester, so we don't need the server anymore. Thanks for the idea, it's much simpler now. |
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
conans/client/tools/net.py
Outdated
| out.warn("Could not download from the url {}.".format(url_it)) | ||
| if (index + 1) == len(url): | ||
| raise ConanException("All {} URLs have failed: {}.".format(len(url), | ||
| str(error))) |
There was a problem hiding this comment.
This is the typical use-case for a for/else loop, but I'm not sure if we want that logic in the codebase.
There was a problem hiding this comment.
I think every failed URL should output the error message too. And the last one, maybe just raise the exception it got?
There was a problem hiding this comment.
Almost it, but you point makes more sense. For now it raises a warning message, but I'll raise an error message, and a warning message the the retry.
| @@ -122,7 +122,7 @@ def source(self): | |||
| """ % http_server.port) | |||
| client.save({"conanfile.py": conanfile}) | |||
| client.run("source .", assert_error=True) | |||
| self.assertIn("ConanException: md5 signature failed", client.out) | |||
| self.assertIn("ConanException: All 1 URLs have failed: md5 signature failed", client.out) | |||
There was a problem hiding this comment.
I don't like having this error message when you only provide 1 url, it makes the output a bit more confusing to the user.
There was a problem hiding this comment.
I see, I'll filter it raise a specific message when there is only one URL.
conans/client/tools/net.py
Outdated
| out.warn("Could not download from the url {}.".format(url_it)) | ||
| if (index + 1) == len(url): | ||
| raise ConanException("All {} URLs have failed: {}.".format(len(url), | ||
| str(error))) |
There was a problem hiding this comment.
I think every failed URL should output the error message too. And the last one, maybe just raise the exception it got?
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Co-Authored-By: Javier G. Sogo <jgsogo@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
|
@memsharded I just updated the error messages, I think it's better now. |
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
|
UPDATE: Updated documentation conan-io/docs#162 |
conans/client/tools/net.py
Outdated
| message = "Could not download from the URL {}: {}.".format(url_it, str(error)) | ||
| if len(url) == 1: | ||
| raise ConanException(message) | ||
| out.error(message) |
There was a problem hiding this comment.
One question about this line: will someone parse this ERROR as an actual error and think that the command has failed? Is it an error to fail with an URL when you have a mirror/fallback strategy? Printing something to stderr (we are not printing errors to stderr, should we?) is considered a failure from a CLI perspective?
wdyt, @memsharded @uilianries @czoido ?
There was a problem hiding this comment.
This is a very good point, indeed.
Probably should be a warning, not an error. Not a failure from CLI perspective, the command have succeeded if it was able to fallback to a mirror.
There was a problem hiding this comment.
Okay, I'll update it to warning level.
@uilianries , please, change the error to warn (#6679 (comment)), and we'll be done!
Signed-off-by: Uilian Ries <uilianries@gmail.com>
|
@jgsogo done, I've changed to warning level. |
Thanks for the work here, @uilianries . And sorry for all the back and forth
|
@jgsogo thank you for reviewing and making it better! |
Changelog: Feature: Support mirrors for
tools.downloadandtools.get.Docs: conan-io/docs#1623
This PR follows the instructions described in #6349 (comment)
Updated the follow methods:
tools.ftp_downloadcloses #6349
closes #6246
developbranch, documenting this one.Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.