Remove qhttp; replace with curl and qhttpengine #1169

droidmonkey · 2017-11-06T04:46:20Z

Description

This PR completely removes qhttp and replaces with the following packages:

Use curl for http client requests (Custom Icons)
Use qhttpengine for KeePassHTTP server

The custom icon download sequence was significantly improved with code reduction around 60% and no recursion.

Note: This PR requires the debian repositories to be updated with qhttpengine 1.x before merging

Fixes #913

Motivation and context

The primary driver for this change is to align with Debian requirements. Secondarily, we are moving off of an unsupported, flaky library.

How has this been tested?

Manually; automated test cases should be written for this when KeePass-Browser is merged.

Types of changes

✅ Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
✅ My change requires a change to the documentation and I have updated it accordingly.
❌ I have added tests to cover my changes.

droidmonkey · 2017-11-06T05:01:45Z

Please put the review on hold while I figure out these dependency issues...

nathan-osman · 2017-11-07T06:12:02Z

If I may jump in and offer a bit of help... QHttpEngine wasn't added to Ubuntu until Yakkety (16.10) was released. Unfortunately, Travis CI is still using Trusty (14.04) for their default build environment.

I do maintain a PPA with builds for Xenial (16.04) but apparently Travis CI doesn't provide an option for building on Xenial yet (even though it was released over a year and a half ago).

Apparently it may be possible to use Xenial in Docker on Travis CI. I'm not entirely sure how that would work but it seems to be the only option.

nathan-osman · 2017-11-07T06:15:36Z

After giving this some more thought - one other option is to stick with Trusty, add this PPA, and then download/build/install QHttpEngine directly.

I use a similar approach for testing QHttpEngine itself in Travis CI - you might find this useful: https://github.com/nitroshare/qhttpengine/blob/master/.travis.yml

droidmonkey · 2017-11-07T11:55:13Z

Hi @nathan-osman. Unfortunately the distribution hoops and significant difference in version (0.1 vs 1.0) are leading me to have to go back to libmicrohttpd. I didn't realize there was such a difference between version ticks. Would it be possible to smooth out what is already in the downstream repos, especially Debian?

At this point you have the only actively maintained qt http wrapper/lib on the market. I checked out every other c/c++ option and they all fall apart in various areas, mainly in upkeep and distribution. It would be worth a little investment before qt6 comes out with a built-in http solution (hopefully).

nathan-osman · 2017-11-08T03:24:17Z

I'd love to but I don't have package upload rights to Debian so I'm dependent on getting a sponsor to upload new versions of the package. And yes, there were some rather significant changes between versions which makes this more complicated since the new version will probably require a new package name to avoid breaking existing packages.

One option is always just embedding QHttpEngine in your project's source code. It is released under the MIT license so it should be usable pretty much anywhere.

droidmonkey · 2017-11-08T04:10:29Z

One of the main drivers in this move to your engine is Debian requiring us to move the http libraries outside the code base (ie, in a dynamically linked library) to allow for security updates and comply with their policies. We have a line in with the Debian folks, it might be easy to just append a modifier like qhttpengine-1-dev

adolfogc · 2017-11-20T20:50:33Z

@droidmonkey I tried using the following lines in src/http/CMakeLists.txt:

# ...
    if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
      find_package(QHttpEngine REQUIRED CONFIG)
      set(qhttpengine QHttpEngine)
      find_package(CURL REQUIRED)
      set(curl CURL)
    else()
      find_package(qhttpengine REQUIRED CONFIG)
      find_package(curl REQUIRED)
    endif()
# ...

It was able to find QHttpEngine in MSYS2/macOS/Linux (Docker image based on debian:stretch); it wasn't able to find curl though.

phoerious

Only two minor style issues. Apart from that, this looks amazing. +132 −7,008 == 😍
I hope we can solve the dependency problems soon.

phoerious · 2017-11-20T21:09:00Z

src/gui/EditWidgetIcons.cpp

 }

-void EditWidgetIcons::fetchFaviconFromGoogle(const QString& domain)
+#ifdef WITH_XC_HTTP
+size_t writeCurlResponse(char *ptr, size_t size, size_t nmemb, void *data)


Pointer operator belongs to type.

phoerious · 2017-11-20T21:09:39Z

src/gui/EditWidgetIcons.cpp

-        resetFaviconDownload();
-        emit messageEditEntry(tr("Unable to fetch favicon."), MessageWidget::Error);
-    }
+    QByteArray* res = static_cast<QByteArray*>(data);


No need to shorten variable names like that here.

droidmonkey · 2017-11-21T03:26:46Z

@adolfogc careful, the qhttpengine located in the linux distros is the old version and incompatible with the v1.0 that I coded against. Working with the package maintainer to fix that issue.

adolfogc · 2017-11-21T16:38:30Z

@droidmonkey

Working with the package maintainer to fix that issue.

Great!

ArchangeGabriel · 2018-01-23T19:27:33Z

What the status of this PR? Is it still relevant with the HTTP module being dropped in the (quite) near future? Maybe the curl part is still important?

phoerious · 2018-01-23T20:18:29Z

Yes, this is still scheduled for 2.3. qhttp is just too buggy.

yan12125 · 2018-02-02T14:56:23Z

Hello, great to see there's an update. I have a question and feedback:

About qhttpengine:

Unfortunately the distribution hoops and significant difference in version (0.1 vs 1.0) are leading me to have to go back to libmicrohttpd

Is there still a plan to go back to libmicrohttpd? I'm not sure if I should create a qhttpengine package for MacPorts (I'm the maintainer of KeePassXC at MacPorts).

About curl:

Just like the case in #1169 (comment), on Arch Llinux I need to change find_package(curl REQUIRED) to find_package(CURL REQUIRED) so that the curl package can be found. I guess it's because the relevant CMake module is called FindCURL.cmake instead of Findcurl.cmake [1].

[1] https://gitlab.kitware.com/cmake/cmake/blob/master/Modules/FindCURL.cmake

droidmonkey · 2018-02-02T17:21:57Z

libmicrohttpd is off the table, I want to stick with the Qt solution. Please do build a qhttpengine package for macports.

I'll fix the package finder for CURL

yan12125 · 2018-02-05T06:20:17Z

Hello, could you rebase this? The yubikey fix is merged into develop (d911987) and leads to a conflict.

droidmonkey · 2018-02-06T01:30:15Z

closing in favor of a new PR that only implements CURL.

droidmonkey added this to the v2.3.0 milestone Nov 6, 2017

droidmonkey requested a review from a team November 6, 2017 04:46

droidmonkey force-pushed the refactor/qhttpengine branch 2 times, most recently from 68f7a2b to 150f9f2 Compare November 6, 2017 04:58

droidmonkey force-pushed the refactor/qhttpengine branch from 150f9f2 to cfb7aa8 Compare November 16, 2017 02:37

droidmonkey mentioned this pull request Nov 20, 2017

Update debian and ubuntu packages nitroshare/qhttpengine#24

Closed

phoerious requested changes Nov 20, 2017

View reviewed changes

droidmonkey force-pushed the refactor/qhttpengine branch from cfb7aa8 to 52a5666 Compare November 27, 2017 03:31

droidmonkey force-pushed the refactor/qhttpengine branch from 52a5666 to 346ff5a Compare December 8, 2017 03:23

droidmonkey force-pushed the refactor/qhttpengine branch from 346ff5a to c7d2f1f Compare December 28, 2017 22:55

phoerious mentioned this pull request Jan 12, 2018

Isolate/Outsource stuff with network requests #1377

Closed

droidmonkey force-pushed the refactor/qhttpengine branch from c7d2f1f to 4ff4e9b Compare January 14, 2018 15:35

adolfogc mentioned this pull request Jan 15, 2018

Add initial AppVeyor support #1380

Merged

droidmonkey added 3 commits January 30, 2018 22:07

Ensure yubikey libs are dynamic; bump to c99

6ec24c6

Remove qhttp; replace with curl and qhttpengine

2211f69

Added WITH_XC_NETWORKING back in

b21783e

droidmonkey force-pushed the refactor/qhttpengine branch from 4ff4e9b to b21783e Compare January 31, 2018 04:23

droidmonkey closed this Feb 6, 2018

droidmonkey deleted the refactor/qhttpengine branch February 6, 2018 01:30

droidmonkey mentioned this pull request Feb 6, 2018

Replace qhttp client with curl for favicon downloading #1460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove qhttp; replace with curl and qhttpengine #1169

Remove qhttp; replace with curl and qhttpengine #1169

droidmonkey commented Nov 6, 2017 •

edited

Loading

droidmonkey commented Nov 6, 2017

nathan-osman commented Nov 7, 2017

nathan-osman commented Nov 7, 2017 •

edited

Loading

droidmonkey commented Nov 7, 2017 •

edited

Loading

nathan-osman commented Nov 8, 2017 •

edited

Loading

droidmonkey commented Nov 8, 2017

adolfogc commented Nov 20, 2017

phoerious left a comment

phoerious Nov 20, 2017

phoerious Nov 20, 2017

droidmonkey commented Nov 21, 2017

adolfogc commented Nov 21, 2017

ArchangeGabriel commented Jan 23, 2018

phoerious commented Jan 23, 2018

yan12125 commented Feb 2, 2018

droidmonkey commented Feb 2, 2018 •

edited

Loading

yan12125 commented Feb 5, 2018

droidmonkey commented Feb 6, 2018

Remove qhttp; replace with curl and qhttpengine #1169

Remove qhttp; replace with curl and qhttpengine #1169

Conversation

droidmonkey commented Nov 6, 2017 • edited Loading

Description

Motivation and context

How has this been tested?

Types of changes

Checklist:

droidmonkey commented Nov 6, 2017

nathan-osman commented Nov 7, 2017

nathan-osman commented Nov 7, 2017 • edited Loading

droidmonkey commented Nov 7, 2017 • edited Loading

nathan-osman commented Nov 8, 2017 • edited Loading

droidmonkey commented Nov 8, 2017

adolfogc commented Nov 20, 2017

phoerious left a comment

Choose a reason for hiding this comment

phoerious Nov 20, 2017

Choose a reason for hiding this comment

phoerious Nov 20, 2017

Choose a reason for hiding this comment

droidmonkey commented Nov 21, 2017

adolfogc commented Nov 21, 2017

ArchangeGabriel commented Jan 23, 2018

phoerious commented Jan 23, 2018

yan12125 commented Feb 2, 2018

droidmonkey commented Feb 2, 2018 • edited Loading

yan12125 commented Feb 5, 2018

droidmonkey commented Feb 6, 2018

droidmonkey commented Nov 6, 2017 •

edited

Loading

nathan-osman commented Nov 7, 2017 •

edited

Loading

droidmonkey commented Nov 7, 2017 •

edited

Loading

nathan-osman commented Nov 8, 2017 •

edited

Loading

droidmonkey commented Feb 2, 2018 •

edited

Loading