search

qgis / QGIS

QGIS is a free, open source, cross platform (lin/win/mac) geographical information system (GIS)
https://qgis.org
GNU General Public License v2.0
10.65k stars 3.01k forks source link

Re-sync external o2 library #59541

Closed nyalldawson closed 19 hours ago

nyalldawson commented 1 day ago

Keep a few downstream patches:

Funded by RGD Savoie Mont Blanc

github-actions[bot] commented 1 day ago

🪟 Windows builds

Download Windows builds of this PR for testing. Debug symbols for this build are available here. (Built from commit 6fbbf6e81e0830749a3a155efa28205ab2952502)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing. (Built from commit 6fbbf6e81e0830749a3a155efa28205ab2952502)

m-kuhn commented 1 day ago

Did we open upstream PR's for the downstream patches?

nyalldawson commented 1 day ago

@m-kuhn good question.

Of the remaining patches here's my thoughts

Oauth2 PKCE initial support (https://github.com/qgis/QGIS/commit/76df15690c5b3e7fd2f97177cd8e743546821f07)

Only part of the implementation is in the o2 library, the rest is in QGIS specific code

Hide more debug noise (https://github.com/qgis/QGIS/commit/1f91895b1e8d4ca8dd2bb24b0ea35f5bcb425a96)

QGIS specific requirement -- I don't think it's likely to be wanted upstream. We just comment out all the many qDebug calls scattered throughout the library.

If an o2 auth refresh reply contains an error message, then the refresh was NOT successful (https://github.com/qgis/QGIS/commit/9af5a531b1a9c4b452b98c0a10387aefa850edee)

Fairly specific to ESRI servers -- I don't think there's a wider need for this.

Ensure correct thread locale QgsNetworkAccessManager is used during o2 requests (https://github.com/qgis/QGIS/commit/9cde65457b3717a74e32277c843a2945eb4f1221)

Very QGIS specific change.

Find QtKeychain by its cmake target (https://github.com/qgis/QGIS/commit/b9e859f0ed1d9d783c0a7b4f28e3d73ac21572f6) Allow compiling o2 with Qt6 (https://github.com/qgis/QGIS/commit/6fbe9a4fb3a29bbbd32987153303181d3d4362cd)

Maybe there's some value in upstreaming these, but other Qt 6 related patches are rotting upstream for > 12 months : https://github.com/pipacs/o2/pulls

So in short, I'm reluctant to bother :stuck_out_tongue:

m-kuhn commented 1 day ago

I am sure we could find a way to properly upstream the QGIS specific patches in a more generic way and the qDebug changes would not be required if consumed from a dependency (which we can assume will be normally built in release mode when installed to the system).

But ... with an unresponsive upstream (3 years since last commit) I can understand that the motivation to go the extra mile is rather low and it's more a question if we adopt the code completely or if a new fork would be better suited.

🟢

Thanks for the detailed listing of the patches in here !