Make Program Updater choose the same build for download

[sledgehammer999]

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.

[glassez]

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.

[glassez]

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.

[sledgehammer999]

Is there an official way of using QT_VERSION_MAJOR ? I can't find any reference in the Qt docs. I want to do something like this:

#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

[Chocobo1]

Is there an official way of using QT_VERSION_MAJOR ?

https://doc.qt.io/qt-6/qtversionchecks-qtcore-proxy.html#QT_VERSION_CHECK

[sledgehammer999]

Is there an official way of using QT_VERSION_MAJOR ?

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. eg qt5, qt6, qt7 I don't care about the version inbetween. AFAICT we aren't supposed to access QT_VERSION_MAJOR directly, correct? Or the QtCore/qconfig.h header, correct?

[Chocobo1]

There is also QT_VERSION and you can extract MAJOR component from it. Probably it isn't going to be straightforward.

AFAICT we aren't supposed to access QT_VERSION_MAJOR directly, correct? Or the QtCore/qconfig.h header, correct?

I'm not sure, if CI accepts it then it is probably ok.

[sledgehammer999]

I have pushed changes with a bunch of preprocessor macros. But I am not good with macros. Is there any way to do:

const QString OS_TYPE = u<preprocessor defined string>_s;

If not, what's the next best thing? QStringLiteral?

[sledgehammer999]

Nevermind, I think I have fixed the macro madness. It should work now.

[glassez]

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.

[sledgehammer999]

even if it leads to do some things in runtime instead

Cool. Done. I just wish we had full string composition available in constexpr. Probably can be done if you jump through enough hoops, but I don't want to devote more time to this.

[glassez]

@sledgehammer999, @Chocobo1 Wouldn't it make sense to always include Qt+libtorrent info in installer name (i.e. don't omit it for "default" variant)?

[sledgehammer999]

Fixed comments. Hopefully the var name is satisfactory now.

Wouldn't it make sense to always include Qt+libtorrent info in installer name (i.e. don't omit it for "default" variant)?

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 https://github.com/qbittorrent/qBittorrent/commit/93204a63ad244aec14ed0676dc82a3884c1e3aad for v4.5x -> v4.6x.

[glassez]

Wouldn't it make sense to always include Qt+libtorrent info in installer name (i.e. don't omit it for "default" variant)?

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.

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:

1. Default build will always suggest new default one (regardless of the libraries used).
2. Non-default build will always suggest matched new non-default build.

How is it supposed to behave if the new release does not provide a suitable build?

[glassez]

Ideally, I would suggest doing it this way:

1. qBittorrent requests information about the update, passing the parameters of the current build to the server, for example, https://qbittorrent.org/update?os=windows&qt=6&libtorrent=2.0
2. The server suggests a suitable update option based on these parameters, or rejects the request if there are no suitable options (for example, as in the above case, when the update drops support for the OS used, etc.)

[sledgehammer999]

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:

1. Default build will always suggest new default one (regardless of the libraries used).

2. Non-default build will always suggest matched new non-default build.

That's my explicit intention for the workflow.

How is it supposed to behave if the new release does not provide a suitable build?

Ideally, we would foresee this and the last release of the compatible version would have internal checks to disable update lookup.

Ideally, I would suggest doing it this way:

1. qBittorrent requests information about the update, passing the parameters of the current build to the server, for example, `https://qbittorrent.org/update?os=windows&qt=6&libtorrent=2.0`

2. The server suggests a suitable update option based on these parameters, or rejects the request if there are no suitable options (for example, as in the above case, when the update drops support for the OS used, etc.)

Honestly, I don't want to transfer logic to the server. It might complicate things. It might open an attack vector. It might help user data leaks (they will be sending their config essentially). I want to keep it simple (from a dev POV).

[glassez]

How is it supposed to behave if the new release does not provide a suitable build?

Ideally, we would foresee this and the last release of the compatible version would have internal checks to disable update lookup.

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.)

[sledgehammer999]

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.

You mean that the non-default install will not fallback to the new default, which is compatible/same, right? That was what I was thinking right now too. I want to solve it manually this way: The last release will include migration code for the non-default build to offer the new default. I suppose this will be rare occasions, so it doesn't make sense to code complex logic in the updater.

[glassez]

@sledgehammer999 It remains to evaluate one more idea to complete this brainstorming session. What about disabling update checking for all "non-default" builds altogether?

[sledgehammer999]

What about disabling update checking for all "non-default" builds altogether?

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. IMO, it's not expected that non-default builds will not offer update notifications.

[sledgehammer999]

Any news on this?

[glassez]

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.

[sledgehammer999]

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. Any reservations?

[glassez]

Aka the non-default case should encode qt+lt into the variant name.

👍

[qBittUser]

This was added to v4_6_x, but isn't backported to v5_0_x?