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

Use libcurl for HTTP transfers #218

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

gonosztopi
Copy link
Member

@gonosztopi gonosztopi commented Oct 2, 2020

Although commonly downloaded files (server.met, nodes.dat, etc.) are now available via plain HTTP, issues #197 and #193 prove it wasn't always the case. libcurl supports HTTPS for us out of the box.

It is now optional to use libcurl, as it's a new dependency, but this might change in the future.

Since HTTP download was the only piece of neworking code not covered by Boost.Asio, combined with libcurl now allows for completely forgetting about wxWidgets networking.

The patch is working but isn't yet complete, I provide it here mainly for testing purposes. Please report any failures here. The testing branch has been updated with the contents of this pull request.

TODO:

  • Stop transfer if "Cancel" is pressed on the progress dialog.
  • Integrate with supported build systems:
    • Debian
    • Windows (Visual Studio)
    • cmake support

Advantage of libcurl over wxHTTP is that we get HTTPS support out of the box.
Disadvantage is it being another dependency. Therefore using libcurl is optional
now, but strongly encouraged.

Since HTTP download was the last piece of network functions not replaced by
Boost.Asio, using libcurl together with boost makes it possible to forget wxWidgets
networking completely.
Minimum required libcurl version is now 7.32.0, required by CURLOPT_XFERINFOFUNCTION.
Since it's already seven years old, we're not planning to support older versions.
This is mainly a demonstration of how we can select new functionality if available.
First, we have to check the compile-time headers version whether they have the new
functionality or not, to be able to compile it in. Then, we have to check the library
version at runtime because that might very well be different from what we used for
compilation. If both checks succeed then and only then can we use the new interface.
In all other cases we need to revert to use the old interface.
@gonosztopi gonosztopi added this to the v2.3.3 milestone Oct 2, 2020
@gonosztopi gonosztopi self-assigned this Oct 2, 2020
@gonosztopi gonosztopi marked this pull request as draft October 2, 2020 10:59
We use the progress callback to periodically check whether the current download
is requested to be cancelled (i.e. the user pressed the 'Cancel' button, or the
application is exiting). That requires that we always have a progress callback
installed, therefore the minimum required libcurl version is lowered to 7.19.1.
@gonosztopi
Copy link
Member Author

gonosztopi commented Oct 29, 2020

@Vollstrecker could you please take a look on 71e56dd ? I'm still new to debian packaging...

Also, although I set the required minimum version to >= 7.19.1, the binary packages contain a dependency on >= 7.16.2. Is there a way to override that (maybe without knowing the exact name of the dependent binary package)?

@Vollstrecker
Copy link
Collaborator

Even oldoldstable has 7.38, so I would omit the version at all. But it doesn't harm to have it in.

@gonosztopi
Copy link
Member Author

Even oldoldstable has 7.38, so I would omit the version at all. But it doesn't harm to have it in.

Thank you

@sc0w
Copy link
Member

sc0w commented Dec 26, 2020

The https support in wxWidgets is work in progress:

https://github.com/wxWidgets/wxWidgets/pull/977

@sc0w
Copy link
Member

sc0w commented Jan 18, 2021

https://github.com/wxWidgets/wxWidgets/pull/977 was merged !

@gonosztopi
Copy link
Member Author

wxWidgets/wxWidgets#977 was merged !

Thanks, and I see, it uses libcurl on Linux, anyway. However, it won't be available before 3.2.0 and we have problems even with 3.1.x.... not to say we still support 2.8.12. Even if we'll once change to it completely we need a rewrite of our HTTP download logic.

@minnyres
Copy link

minnyres commented May 3, 2022

I can conform it works on Windows, but some improvement to the autotools build system are needed:

  1. Support static libcurl. If libcurl is built as a static library, when I run ./configure, it will complain that libcurl is found but not usable. Adding the option -DCURL_STATICLIB to CXXFLAGS and CFLAGS solves this problem, see simple curl program fails on win10-64/codeblocks/mingw curl/curl#3748.
  2. For some reason, the option -DHAVE_LIBCURL is not included in CPPFLAGS, so the curl headers are not included.

BTW, MinGW GCC will report the error "cannot convert ‘LPCTSTR’ {aka ‘const char*’} to ‘LPCWSTR’ {aka ‘const wchar_t*’}" when aMule is built with boost, pupnp or libcurl, see #275. The workaround is to include the wxwidgets headers before the headers of boost, pupnp and libcurl.

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.

aMule does not support HTTPS git HEAD 3a77afb: unable to fetch servers list
4 participants