-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Make Program Updater choose the same build for download #21257
Make Program Updater choose the same build for download #21257
Conversation
I would make it more flexible. I would define the suffixes separately and then combine them into a final string, e.g. "qbittorrent" + suffixOS + suffixLT. |
You can now take into account the Qt version as well. Sooner or later we will find ourselves at the turn of the versions again, so we will have to support builds based on different branches of Qt. |
Is there an official way of using #ifdef QT_VERSION_MAJOR == 6 && LIBTORRENT_VERSION_MAJOR == 1
// compose default string
#else
// compose string by using QT_VERSION_MAJOR, LIBTORRENT_VERSION_MAJOR, LIBTORRENT_VERSION_MINOR
#endif |
https://doc.qt.io/qt-6/qtversionchecks-qtcore-proxy.html#QT_VERSION_CHECK |
This doesn't fit the bill. I want to test specifically the MAJOR component. And then compose a string on that. |
There is also
I'm not sure, if CI accepts it then it is probably ok. |
I have pushed changes with a bunch of preprocessor macros. const QString OS_TYPE = u<preprocessor defined string>_s; If not, what's the next best thing? QStringLiteral? |
58f71de
to
1ca6db0
Compare
Nevermind, I think I have fixed the macro madness. It should work now. |
Personally I would prefer it to be more readable even if it leads to do some things in runtime instead. As I suggested above several parts of resulting string can be defined separately and then joined at runtime. |
1ca6db0
to
80f8a19
Compare
Cool. Done. |
@sledgehammer999, @Chocobo1 |
80f8a19
to
1085b52
Compare
Fixed comments. Hopefully the var name is satisfactory now.
IMO, I want to have the default variants without the qt/lt encoded in the name to facilitate seemless transition to new major versions. If this means a major OS drop then the last version of the series can have checks disabled as in 93204a6 for v4.5x -> v4.6x. |
1085b52
to
74844ca
Compare
I'm just trying to figure out how it will behave in transitional cases (i.e. when the default build composition is changed). IIRC, it will be like the following:
How is it supposed to behave if the new release does not provide a suitable build? |
Ideally, I would suggest doing it this way:
|
That's my explicit intention for the workflow.
Ideally, we would foresee this and the last release of the compatible version would have internal checks to disable update lookup.
Honestly, I don't want to transfer logic to the server. |
I was mostly referring to the case when a compatible version is still available. For example, the user uses non-default libtorrent-2.0 based build, and in one of the next updates libtorrent-2.0 becomes the default, so corresponding build will not be recognized. (I just want to make sure that this is known and considered to be acceptable.) |
You mean that the non-default install will not fallback to the new default, which is compatible/same, right? |
@sledgehammer999 |
Take for example the current builds we offer. I suspect current lt20 users will start complaining once they realize they have missed a couple of updates. |
Any news on this? |
If it behaves the way you intended, then I have no more questions. I would still prefer it to be implemented as I suggested above (by declaring suffixes separately and then concatenating them). I think it would improve the readability of the code. Also it would be better to declare default Qt and libtorrent versions as constants than using numbers directly. But for now we can leave it as it is. |
I've given this a bit more thought. I want to push a new approach. Essentially I want to make this: if constexpr ((QT_VERSION_MAJOR == 6) && (LIBTORRENT_VERSION_MAJOR == 1))
return BASE_OS;
else if constexpr ((QT_VERSION_MAJOR == 6) && (LIBTORRENT_VERSION_MAJOR > 1))
return u"%1 (lt%2%3)"_s.arg(BASE_OS, QString::number(LIBTORRENT_VERSION_MAJOR), QString::number(LIBTORRENT_VERSION_MINOR));
else if constexpr (QT_VERSION_MAJOR > 6)
return u"%1 (qt%2 lt%3%4)"_s.arg(BASE_OS, QString::number(QT_VERSION_MAJOR), QString::number(LIBTORRENT_VERSION_MAJOR), QString::number(LIBTORRENT_VERSION_MINOR)); if constexpr ((QT_VERSION_MAJOR == 6) && (LIBTORRENT_VERSION_MAJOR == 1))
return BASE_OS;
else
return u"%1 (qt%2 lt%3%4)"_s.arg(BASE_OS, QString::number(QT_VERSION_MAJOR), QString::number(LIBTORRENT_VERSION_MAJOR), QString::number(LIBTORRENT_VERSION_MINOR)); Aka the non-default case should encode qt+lt into the variant name. |
👍 |
74844ca
to
1cc4de2
Compare
We're probably stuck offering the duo of RC_1_2 and RC_2_0 for some time in the future. So hardcode the choices and make the Program Updater choose the variant the user currently uses.
1cc4de2
to
e1492ca
Compare
We're probably stuck offering the duo of RC_1_2 and RC_2_0 for some time in the future. So hardcode the choices and make the Program Updater choose the variant the user currently uses.
Code reviewers: Not sure how to best differentiate between the different levels of #ifdefs. AFAIK identation can't be used here. So I added code comments and empty lines between the blocks.