-
Notifications
You must be signed in to change notification settings - Fork 130
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
update upgrade for Qt6 #1410
update upgrade for Qt6 #1410
Conversation
tsteven4
commented
Feb 15, 2025
- change upgrade url method to https. In the past this caused problems on windows (Download failed: TLS initialization failed. #580, use http for upgrade #587). However, with Qt6 we are shipping plugins for native TLS libraries (windows SChannel, macos Secure Transport) as well as OpenSSL. Qt will fall back on the native plugin if required.
- skip all the use count statistics if the user has disabled them, i.e. babelData_.reportStatistics_ is false.
- implement clazy QString allocation optimizations.
- delete redundant QDateTime constructor.
- use default initializers.
- delete obsolete manual redirection code.
- delete unused variable.
1. change upgrade url method to https. In the past this caused problems on windows (GPSBabel#580, GPSBabel#587). However, with Qt6 we are shipping plugins for native TLS libraries (windows SChannel, macos Secure Transport) as well as OpenSSL. Qt will fall back on the native plugin if required. 2. skip all the use count statistics if the user has disabled them, i.e. babelData_.reportStatistics_ is false. 3. implement clazy QString allocation optimizations. 4. delete redundant QDateTime constructor. 5. use default initializers. 6. delete obsolete manual redirection code. 7. delete unused variable.
I did notice automatic redirection fails for |
Also in the response
At least the version attribute is correct! |
I have exercised this with "static const bool testing = true;" on Windows 11 and was offered the upgrade. |
I have exercised this with "static const bool testing = true;" on macos 15.3.1 and was offered the upgrade. This also worked when I had deleted either the openssl plugin or the secure transport plugin. libopenssl was present in /usr/local/lib from homebrew. |
if (babelData_.reportStatistics_) { | ||
int j = 0; | ||
|
||
for (int i = 0; i < formatList_.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have any other reason to touch this, please consider:
for (const auto& entry : formatList_)
and them making the formatList[i] below into entry...and/or just moving rc and wc into the one place they seem to be used.
If you're tired of looking at this, that's OK, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments. I have lost count of the number of times I have started to modernize the loops in the GUI. I think I will defer this for a more global solution for the GUI.
That was/is special cased because had some versions in play that couldnt handle the redirect so it actually overrides the global (if not https, redirect) rewrite. There's probably a generation of some combination of OS and version that's an island |
Re: the prose being wrong now points to a new page that says we don't have release notes. At some point, I should just give up on the pretense of even offering those. The GitHub history is there for anyone that cares - and it seems that only you and I do. There is probably some overdesign in the XML that's returned vs. what's actually shown to the user. Back in 1.2 I thought about offering translated pages for example, but online and in-browser translation make that seem pretty silly. That 'testing' path you found was exactly to provide an easy one-byte change to allow testing it. It was easier during development than editing registry keys in different ways on different OSes. I'm glad you found it useful ... and more glad that you've confirmed it's all working in modern times. I usually just spot-check my own desktop after an upgrade but don't run around the whole matrix. |
If the GUI (on linux) uses http://gpsbabel.org/upgrade_check.html it still fails. Note the scheme and the subdomain are both changed.
I note curl handles the 301 followed by a 307 without an issue. Qt bug? I'm not sure we care.
|
Thanks. Even if there isn't much there it's nice to not refer to 1.7 and 1.8 anymore. |
regarding the 301,307 redirect failure after the 307 the GUI sends the following. Note that the content-type and content-length headers are present, yet the content is empty. This is with Qt 6.8.2 but I don't think that matters. wireshark has some trouble decoding this frame, it shows the protocol as TLSv1.3 instead of HTTP.
After the 301 the GUI sends a GET without the content* headers and an empty body. But after the 307 the content headers come back.
I don't know that the content* headers cause the timeout, but curl works and it strips them from both redirect generated requests. I can imagine gpsbabel.org is waiting for the 234 bytes of content length. |
The problem with the POST generating a 301, 307 redirect sequence is a regression between Qt 6.6.3 and 6.7.0. It seems to be due to qt/qtbase@0345f07#diff-1747ac73c8af554792736e60577b4c366e9b0408d5b82b8100f995b5aa6d329a Specifically getOperationKeepsBody was introduced. In our case a POST with a body is followed by a 301 redirect, which is followed by a 307 redirect. The 301 redirect executes the code clearing the outgoingData and Content headers. It also changes operation to a GET. When the 307 redirect comes getOperationKeepsBody is true so we don't clear the Content Headers. Note that the redirectRequest is created each time from the originalRequest which is not overwritten by the redirects. Thus, the original headers from the POST are copied for the 307 redirect, but the body has been cleared. I assert that the 307 GET request should not have Content headers. With them a timeout eventually results.
|
As I don't have an Qt account I emailed the author of the commit at qt.io. Hopefully they can address the issue. |
Thanks @wpbrown for https://github.com/wpbrown/openssl-keylog. That was very helpful debugging this issue and played nicely with the Qt Network API (when it is backed by OpenSSL). |