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 warning when downloading recipes from bintray #9416

Merged

Conversation

danimtb
Copy link
Member

@danimtb danimtb commented Aug 12, 2021

Changelog: omit
Docs: omit

  • Refer to the issue that supports this Pull Request: continuation of Remove conan-center default remote #9401
  • 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.

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.

Here you can check the output of an install command:

Configuration:
[settings]
arch=x86_64
arch_build=x86_64
build_type=Release
compiler=Visual Studio
compiler.runtime=MD
compiler.version=16
os=Windows
os_build=Windows
[options]
[build_requires]
[env]

zlib/1.2.8@conan/stable: Retrieving from server 'conan-center' 
zlib/1.2.8@conan/stable: Trying with 'conan-center'...
zlib/1.2.8@conan/stable: WARN: Remote https://conan.bintray.com is deprecated and will be shut down soon.
zlib/1.2.8@conan/stable: WARN: Please use the new 'conancenter' default remote.
zlib/1.2.8@conan/stable: WARN: Add it to your remotes with: conan remote add -i 0 conancenter https://center.conan.io
Downloading conanmanifest.txt
Downloading conanfile.py
Downloading conan_export.tgz
zlib/1.2.8@conan/stable: Downloaded recipe revision 0
Installing package: zlib/1.2.8@conan/stable
Requirements
    zlib/1.2.8@conan/stable from 'conan-center' - Downloaded
Packages
    zlib/1.2.8@conan/stable:3fb49604f9c2f729b85ba3115852006824e72cab - Download

Installing (downloading, building) binaries...
zlib/1.2.8@conan/stable: Retrieving package 3fb49604f9c2f729b85ba3115852006824e72cab from remote 'conan-center' 
Downloading conanmanifest.txt
Downloading conaninfo.txt
Downloading conan_package.tgz
zlib/1.2.8@conan/stable: Package installed 3fb49604f9c2f729b85ba3115852006824e72cab
zlib/1.2.8@conan/stable: Downloaded package revision 0
Aggregating env generators

Warning is only displayed when downloading a recipe from the remote
"""
client = TestClient()
client.run("remote add conan-center https://conan.bintray.com")
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to avoid using the real bintray remote with a test server using the same URL. However, this lead to modifications to the TestServer class and others in the requester...

Anyway, I think it could be useful to have a test that tracks the availability of the remote so we can drop the message once it is no longer available 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, in my opinion, I think we should use a "fake" Bintray URL and check the output:

    client = TestClient()
    fake_bintray_url = "https://myfakebintray.com"
    with patch.object(proxy, "BINTRAY_DEPRECATED_URL", return_value=fake_bintray_url):
         client.run("remote add conan-center %s" % fake_bintray_url)
         # .....

What do you think @danimtb?

EDIT: perhaps that client.run() can be outside of the patch

@danimtb danimtb requested a review from jgsogo August 12, 2021 11:18
@danimtb danimtb added this to the 1.40 milestone Aug 12, 2021
@@ -113,6 +113,11 @@ def _download_recipe(self, layout, ref, output, remotes, remote, recorder):
def _retrieve_from_remote(the_remote):
output.info("Trying with '%s'..." % the_remote.name)
# If incomplete, resolve the latest in server
if "https://conan.bintray.com" in the_remote.url:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should declare that URL as a global variable in this module like:

BINTRAY_DEPRECATED_URL = "https://conan.bintray.com"

# .....

if the_remote.url.startswith(BINTRAY_DEPRECATED_URL):
      # whatever

Then, it should be easier to use it in any test.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Two picky comments

Comment on lines 119 to 120
output.warn("Please use the new 'conancenter' default remote "
"(https://center.conan.io).")
Copy link
Contributor

Choose a reason for hiding this comment

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

For these messages, I really find it useful to provide a copy/paste solution to users:

Suggested change
output.warn("Please use the new 'conancenter' default remote "
"(https://center.conan.io).")
output.warn("Please use the new 'conancenter' default remote. Add it to your remotes with "
"'conan remote add --insert 0 conancenter https://center.conan.io'.")

@@ -113,6 +113,11 @@ def _download_recipe(self, layout, ref, output, remotes, remote, recorder):
def _retrieve_from_remote(the_remote):
output.info("Trying with '%s'..." % the_remote.name)
# If incomplete, resolve the latest in server
if "https://conan.bintray.com" in the_remote.url:
Copy link
Contributor

@jgsogo jgsogo Aug 13, 2021

Choose a reason for hiding this comment

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

When using Bintray, if you clicked the set me up button it didn't show you the https://conan.bintray.com/... URL, but https://bintray.com/conan.... (I cannot remember exactly the path)... and many people had that one configured. Do you remember? Do we want to check also for that URL?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'd say to check fur bintray.com, that should be good enough for both cases, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

the URL https://bintray.com/conan.... will be useless as it is not working (only conan.bintray.com is still operational), so checking for it doesn't make sense as it won't work from the beginning and would never get this warning message

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very good reason indeed 😄

@@ -113,6 +113,11 @@ def _download_recipe(self, layout, ref, output, remotes, remote, recorder):
def _retrieve_from_remote(the_remote):
output.info("Trying with '%s'..." % the_remote.name)
# If incomplete, resolve the latest in server
if "https://conan.bintray.com" in the_remote.url:
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'd say to check fur bintray.com, that should be good enough for both cases, isn't it?

@memsharded memsharded merged commit f16ff88 into conan-io:develop Aug 23, 2021
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.

None yet

6 participants