Skip to content
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

Raise minimum libtorrent version to 1.2.14 (2.0.4) #15093

Merged
merged 1 commit into from
Jun 30, 2021

Conversation

FranciscoPombal
Copy link
Member

@FranciscoPombal FranciscoPombal commented Jun 12, 2021

  • Also update vcpkg to latest commit: includes libtorrent 1.2.14,
    Qt 5.15.2, and Qt 6.1.1

Split from #15090.

@FranciscoPombal FranciscoPombal added the Build system Issue with the build configuration or scripts (but not the code itself) label Jun 12, 2021
@FranciscoPombal FranciscoPombal added the CI Issues/PRs related to CI label Jun 12, 2021
@glassez
Copy link
Member

glassez commented Jun 12, 2021

update vcpkg to latest commit: includes libtorrent 1.2.14,
Qt 5.15.2, and Qt 6.1.0

This looks good as it adds support for Qt6.
But why do we really require raising minimum supported libtorrent versions?

@FranciscoPombal
Copy link
Member Author

@glassez

This looks good as it adds support for Qt6.

👍

But why do we really require raising minimum supported libtorrent versions?

We've been doing so for a while now (minor versions, e.g. .12, .13, and now .14), as part of our policy that it doesn't really make sense to allow building with previous versions minor that contain known bugs.

@glassez
Copy link
Member

glassez commented Jun 12, 2021

our policy that it doesn't really make sense to allow building with previous versions minor that contain known bugs.

#14189 (comment)
The policy is a little different from how you describe it. The main idea is not to drop previous versions as soon as a new one is released, but only if its support would require conditional code separation (for example, to add support for some new feature) or some other cares.
I didn't notice that this raising of libtorrent version affected our source code.

@xavier2k6
Copy link
Member

xavier2k6 commented Jun 12, 2021

EDIT: There's a fix in libtorrent 1.2.13 that resolves crashes ref.:#14192.

There's a fix in libtorrent 1.2.14 that resolves crashes ref.:#14474.

@FranciscoPombal
Copy link
Member Author

@glassez

OK, but isn't stuff like this a good enough reason?

There's a fix in libtorrent 1.2.14 that resolves crashes ref.:#14474.

@glassez
Copy link
Member

glassez commented Jun 13, 2021

OK, but isn't stuff like this a good enough reason?

There's a fix in libtorrent 1.2.14 that resolves crashes ref.:#14474.

This is a good reason to make official releases based on it and recommend it to others.
But I still don't think we should deliberately prevent the use of previous versions if our code is technically compatible with them. If someone has sufficient reasons to use a version of libtorrent that has such "known bugs", then this is their concern (maybe they are not even affected by this bug).

@FranciscoPombal
Copy link
Member Author

@glassez

This is a good reason to make official releases based on it and recommend it to others.
But I still don't think we should deliberately prevent the use of previous versions if our code is technically compatible with them. If someone has sufficient reasons to use a version of libtorrent that has such "known bugs", then this is their concern (maybe they are not even affected by this bug).

Quoting from #14189 (comment), emphasis mine:

So we decided to increase the minimum supported version of libtorrent as soon as we use some feature from it (or it has a significant fix/improvement).

"Preventing a known crash" sounds like a "significant fix" to me.

If someone has sufficient reasons to use a version of libtorrent that has such "known bugs", then this is their concern (maybe they are not even affected by this bug).

Exactly - if someone has a specific enough use case that they prefer using a previous, worse version with known bugs, they are on their own, and we don't have to support them.

Do you have any real reason for the minimum to be 1.2.13 and not 1.2.14?
The only one I could think of, playing devil's advocate against my own point of view, would be availability (as in, availability in package managers of distros such as Ubuntu). However, that went out the window a long time ago, since Ubuntu has been very slow in updating libtorrent in its repos.
We've been pretty much assuming for a long time now that users are either compiling libtorrent from source or using some kind of "bleeding edge" distro that provides updated packages quickly.

We should keep moving forward unless there are good reasons to stay behind, and I can't see any.

@glassez
Copy link
Member

glassez commented Jun 14, 2021

We should keep moving forward unless there are good reasons to stay behind, and I can't see any.

Please do it (move forward). No one bothers you. But let someone to stay behind.

Do you have any real reason for the minimum to be 1.2.13 and not 1.2.14?

I would prefer that build system requirements primarily reflect the logical compatibility of the code.

@FranciscoPombal
Copy link
Member Author

@glassez

I would prefer that build system requirements primarily reflect the logical compatibility of the code.

So we should users shoot themselves in the foot by allowing to build against libtorrent 1.2.13 by default? This does not make sense. Again, if someone really needs to build against 1.2.13, they will have no problems editing the build script to make it happen. For everyone else, we should set the requirements to something that does not have known bugs.

"Logical compatibility of the code" is too narrow of a criterion, and contradicts your previous statement:

So we decided to increase the minimum supported version of libtorrent as soon as we use some feature from it (or it has a significant fix/improvement).

Imagine if this were a security issue with an assigned CVE. Would you still be against bumping the version just because the previous insecure code is still "logically compatible"? We should let our dependency requirements protect users against building against versions with known defects, unless there is some bigger trade-off at stake (which is not the case here).

It would be helpful if you could provide logical reasons to stay behind, other than saying "But let someone to stay behind.". I even played devil's advocate in the previous post to try to elicit such an answer from you. Just why? It really boggles my mind why you (and sometimes others) have so much inertia for any kind of upgrade/version bump, especially when this demonstrably fixes real-world problems.

@glassez glassez requested review from a team June 15, 2021 06:23
@userdocs
Copy link

My mistake. It does work with configure/qmake. I I guess I was thinking of older versions.

configure: error: Package requirements (libtorrent-rasterbar >= 1.2.13) were not met:

Package dependency requirement 'libtorrent-rasterbar >= 1.2.13' could not be satisfied.
Package 'libtorrent-rasterbar' has version '1.2.10.0', required version is '>= 1.2.13'

@FranciscoPombal
Copy link
Member Author

@userdocs

My mistake. It does work with configure/qmake. I I guess I was thinking of older versions.

configure: error: Package requirements (libtorrent-rasterbar >= 1.2.13) were not met:

Package dependency requirement 'libtorrent-rasterbar >= 1.2.13' could not be satisfied.
Package 'libtorrent-rasterbar' has version '1.2.10.0', required version is '>= 1.2.13'

Wrong thread?

@xavier2k6
Copy link
Member

Wrong thread?

@FranciscoPombal He deleted his previous comment....

@userdocs
Copy link

Yes I removed it. It was a mistake by me for thinking configure/qmake did not check for the libtorrent version but I had only really tried with older versions. So I tested it with master + v1.2.10 and it worked. Should have done that before I commented I guess. Sorry.

@glassez
Copy link
Member

glassez commented Jun 16, 2021

So we should users shoot themselves in the foot by allowing to build against libtorrent 1.2.13 by default?

Why by default? We just allow to build against libtorrent 1.2.13 whereas official releases are usually built with latest libtorrent.

This does not make sense.

It doesn't matter if it contains nothing but your prohibition, especially since if someone really needs to build against 1.2.13, they will have no problems editing the build script to make it happen.

"Logical compatibility of the code" is too narrow of a criterion

"Logical compatibility of the code" is only reliably dependency. In this case, the libtorrent version requirement means that earlier versions are just incompatible with the current qBittorrent code.
Otherwise, it does not make any distinction between these concepts (code incompatibility and the presence of known bugs).

For everyone else, we should set the requirements to something that does not have known bugs.

But it can have "yet unknown bugs/regressions" which may be even more terrible than previously known. So we can't make such a decision based only on some "known bug" unless there is such a serious error that makes it completely unusable. And even so, I don't see the point of this "security by obscurity". After all, "build system" is not intended for end users, but primarily for developers etc., and for them it can often make sense to build it even with a deliberately non-working dependency.

So I would prefer to stick to the concept of "code compatibility" until the majority votes to "require only the latest libtorrent releases".

@FranciscoPombal
Copy link
Member Author

FranciscoPombal commented Jun 16, 2021

@glassez

Why by default? We just allow to build against libtorrent 1.2.13 whereas official releases are usually built with latest libtorrent.

The main target audience for the build system is developers/contributors, users who want the very latest qBittorrent version, or distributions that want to package it. By stating that 1.2.13 is the minimum required version, we are signaling that it is OK to build with this version. It does not make sense to do so if we know that it is broken.

It doesn't matter if it contains nothing but your prohibition, especially since if someone really needs to build against 1.2.13, they will have no problems editing the build script to make it happen.

I'm not sure what you mean by this, but I'm not "prohibiting" anyone. If someone wants to build with an outdated version when there are clearly superior versions available, they are more than free to make their own patches.

"Logical compatibility of the code" is only reliably dependency. In this case, the libtorrent version requirement means that earlier versions are just incompatible with the current qBittorrent code.
Otherwise, it does not make any distinction between these concepts (code incompatibility and the presence of known bugs).

You merely explained what "Logical compatibility of the code" is. You did not address my argument: that "logical compatibility of the code" was not suitable as the only criterion when deciding whether or not to bump the minimum required version.

But it can have "yet unknown bugs/regressions" which may be even more terrible than previously known.
So we can't make such a decision based only on some "known bug" unless there is such a serious error that makes it completely unusable.

There is a known bug right now that makes qBittorrent unusable for the user that reported it ("After I minimize qbittorrent window about 10 minutes, I can't restore qbittorrent window."), and of course those who also run into the same bug but don't report it. Feel free to look at the changelog for 1.2.14, I think the changes of a regression anywhere are close to 0. If there is indeed some regression lurking, the burden of proof is on you (or others who suffer from these supposed regressions) to show it. You are just speculating about how 1.2.14 might be worse, while currently there is proof that as far as we know, it is better.

By your logic, we would never upgrade anything, since we would always be more fearful of the bugs we don't know than the ones we do... how is software supposed to move forward? Perhaps we should just cancel all our plans to upgrade to libtorrent 2.0.x due to too many "yet unknown bugs/regressions".

And even so, I don't see the point of this "security by obscurity". After all, "build system" is not intended for end users, but primarily for developers etc.,

I don't understand what you mean by "security by obscurity" here, I never mentioned that, and this is not about security. It's about specifying sane requirements and not betraying the expectations of the end users of our build scripts.

and for them it can often make sense to build it even with a deliberately non-working dependency.

Then those developers are doing some weird experiment, they know what they are doing and they are free to edit the scripts. My focus is on the majority of users and distributions who build from source, to whom we are giving the "all clear" to use the build scripts as-is, despite knowing that the bugs exist with the minimum versions that we specify.

So I would prefer to stick to the concept of "code compatibility" until the majority votes to "require only the latest libtorrent releases".

This is a misrepresentation of my position. I never said we should "require only the latest libtorrent releases", not even "require only the latest libtorrent point releases". I'm simply saying that it makes sense bumping the minimum required version not only for code compatibility reasons, but also if there are known bugs that can be trivially fixed via this upgrade.


I will again quote your comment (#14189 (comment)), which you are severely contradicting at this point:

First, previously, this often led to completely idiotic behavior on our part, when we delayed a release in order to wait for the next release of libtorrent, which contains important improvements/fixes, but at the same time we continue to officially support a bunch of previous versions, in fact, having "known bugs", which in addition requires a lot of conditionally included code and additional concerns about it.
(...)
So we decided to increase the minimum supported version of libtorrent as soon as we use some feature from it (or it has a significant fix/improvement).

Note the in addition, which means you do recognize that "requiring a lot of conditionally included code and additional concerns about it." is not the only problem.

@userdocs
Copy link

userdocs commented Jun 16, 2021

It feels weird to this this point argued when from my perspective it would be better to create a proper release setup for Window/Mac/Ubuntu(and debian) taking full advantage of GitHub actions. Then the minimum version is linked to specific release and version increments, no looking back mindset, and makes sense? Until you do that and keep offloading Linux builds to the DIY realm it is a grey area and that is why it does not need to be done, not in this context.

I am hoping my matrix build will lead toward a proper github action based release system where github does the heavy lifting.

#15061

You could have easily released a 4.3.5/1.2.14 and raised the minimum instead of going round in circles here.

@glassez
Copy link
Member

glassez commented Jun 16, 2021

Note the in addition, which means you do recognize that "requiring a lot of conditionally included code and additional concerns about it." is not the only problem.

Actually, this is the essence of the statement (I just can't convey all the subtleties of my idea in English). When this was said, I was mainly concerned about the cleanliness of the code.

I never said we should "require only the latest libtorrent releases", not even "require only the latest libtorrent point releases".

This pleases me.

I'm simply saying that it makes sense bumping the minimum required version not only for code compatibility reasons, but also if there are known bugs that can be trivially fixed via this upgrade.

If I agree with you, won't it untie your hands to treat each new version of libtorrent as containing enough fixes to increase the requirements?

Personally, I can perceive libtorrent as a more integral part of the qBittorrent than its other dependencies. But in this case, we should have a more rigid coupling (up to a specific commit, i.e. a certain release of qBittorrent corresponds to a certain commit of libtorrent), but this is impossible to implement unless we merge its code.

By your logic, we would never upgrade anything, since we would always be more fearful of the bugs we don't know than the ones we do... how is software supposed to move forward?

Sorry, it's your logic, not mine.
I am talking about minimal supported version, not maximal one. It doesn't mean we don't move forward.

I don't intend to get involved in this bickering anymore, as long as there are only two opinions here. If someone else approves of this changes, I won't mind. Moreover, if the majority decides to always "require only the latest libtorrent releases", I will also agree with this.

@glassez
Copy link
Member

glassez commented Jun 17, 2021

Well, it looks like I'm the one who's the devil's advocate on this topic.
After all, I really believe that we should use the most recent version of libtorrent (from a supported branch). The only limiting factor has always been the issue of its availability on those target platforms where shared dependencies are usually used. Even if we do not take this into account, but simply require the latest version of libtorrent, how should we officially treat those who for some reason want (or are forced) to use one of the previous versions? And yet... how can we specify the line that separates compatible, but simply unsupported libtorrent versions, and incompatible ones? Or should we not care about it at all?

@xavier2k6
Copy link
Member

@FranciscoPombal @glassez

May I suggest.......(compromise?)

GHA/CI Test Suites etc.

Continue raising libtorrent version to 1.2.14 (2.0.4) or whatever latest release is available/supported by those systems.

qBittorrent Code:

libtorrent 1.2.x Series - leave as-is. (1.2.13)
libtorrent 2.x.x Series - Raise, as there's been no official release yet & support is still a W.I.P.

There's been fixes in libtorrent 1.2.14 that we know fix crashes but as @glassez has said the official builds use the latest available build/commit(so perhaps there should be a push to release one more build from 4.3 branch??)........otherwise we would have to raise Boost's version to 1.74/1.75 as well since we know 1.74 brought things to the forefront (crashes/url seeds)

We use libtorrent' minimum guideline for that.....which for 2.x.x stands at 1.66 so we can now raise it from 1.65
(we couldn't do it before due to certain linux support, which is no longer relevant as the version was dropped.)

@xavier2k6
Copy link
Member

xavier2k6 commented Jun 18, 2021

@FranciscoPombal Qt 6.1.1 for VCPKG should hopefully be merged soon - so maybe no harm to wait......

ref.: microsoft/vcpkg#18320

@glassez
Copy link
Member

glassez commented Jun 21, 2021

Personally, I can perceive libtorrent as a more integral part of the qBittorrent than its other dependencies.

Well, apparently a stricter restriction of official support for outdated libtorrent versions is a necessary measure. Otherwise, we will not be able to rely on fixes/improvements achieved only by changing the libtorrent. Or we will have to declare them as "Fix something (requires libtorrent v1.2.14+)".
In general, let the build script reflect the official configuration.

P.S. The above applies only to libtorrent. The policy on other dependencies requires a separate discussion.
P.P.S. The above is only my personal opinion. Someone else can have another reasons to support more libtorrent versions.

glassez
glassez previously approved these changes Jun 21, 2021
@FranciscoPombal
Copy link
Member Author

@glassez

You should also check to see if there is any other affected code.

Done.

@FranciscoPombal
Copy link
Member Author

I'm guessing the dependency bundle for Appveyor needs to be updated...

@xavier2k6

This comment has been minimized.

@FranciscoPombal
Copy link
Member Author

@glassez

To address some of your earlier statements, well...

Well, it looks like I'm the one who's the devil's advocate on this topic.
After all, I really believe that we should use the most recent version of libtorrent (from a supported branch). The only limiting factor has always been the issue of its availability on those target platforms where shared dependencies are usually used. Even if we do not take this into account, but simply require the latest version of libtorrent, how should we officially treat those who for some reason want (or are forced) to use one of the previous versions? And yet... how can we specify the line that separates compatible, but simply unsupported libtorrent versions, and incompatible ones? Or should we not care about it at all?

Well, apparently a stricter restriction of official support for outdated libtorrent versions is a necessary measure. Otherwise, we will not be able to rely on fixes/improvements achieved only by changing the libtorrent. Or we will have to declare them as "Fix something (requires libtorrent v1.2.14+)".
In general, let the build script reflect the official configuration.

P.S. The above applies only to libtorrent. The policy on other dependencies requires a separate discussion.
P.P.S. The above is only my personal opinion. Someone else can have another reasons to support more libtorrent versions.

...looks like you answered your own questions. I don't think I could have said it better myself. Looks like we're on the same page after all.

Just one small nitpick:

Personally, I can perceive libtorrent as a more integral part of the qBittorrent than its other dependencies. But in this case, we should have a more rigid coupling (up to a specific commit, i.e. a certain release of qBittorrent corresponds to a certain commit of libtorrent), but this is impossible to implement unless we merge its code.

I assume you're hinting at using git submodules or a similar solution. I disagree with such dependency vendoring, unless it is absolutely required. I think the mechanisms we have now are good enough.

@sledgehammer999
Copy link
Member

I don't feel like the bump to 1.2.14 is justified. More or less the reasons have been already stated by @glassez
The bump to 2.0.4 might be ok. I mean, it is still a moving target for us until release. And AFAIK Ubuntu, Debian, and Arch haven't packaged RC_2_0 yet. When they do, I suppose it will be done for the most recent version available.

glassez
glassez previously approved these changes Jun 27, 2021
@FranciscoPombal
Copy link
Member Author

@sledgehammer999

I don't feel like the bump to 1.2.14 is justified. More or less the reasons have been already stated by @glassez
The bump to 2.0.4 might be ok. I mean, it is still a moving target for us until release.

You should read @glassez's posts again (especially the later ones), he actually agrees that we should upgrade to 1.2.14 (and upgrade libtorrent versions frequently in general, since it is such a core component of qBittorrent).

And AFAIK Ubuntu, Debian, and Arch haven't packaged RC_2_0 yet. When they do, I suppose it will be done for the most recent version available.

At this point, we have stopped caring what libtorrent version any distro packages a long time ago (except perhaps Arch) - they are far too slow. Even the most recent Ubuntu development version is still on 1.2.9, which is too old and is missing critical fixes for issues which were reported here and in the libtorrent issue tracker many times.

For Ubuntu and Debian, we should focus on maintaining a high-quality alternative installation source, such as the PPA.

@sledgehammer999
Copy link
Member

Sorry, I trashed this PR when I merged the travis removal PR. Can you rebase?

I won't hold this PR back for the 1.2.14 bump.

- Also update vcpkg to latest commit: includes libtorrent 1.2.14,
Qt 5.15.2, and Qt 6.1.1
@FranciscoPombal
Copy link
Member Author

@sledgehammer999

Sorry, I trashed this PR when I merged the travis removal PR. Can you rebase?

I won't hold this PR back for the 1.2.14 bump.

👍, rebased.

@sledgehammer999
Copy link
Member

Actually I would drop support for libtorrent-1.2 in qBittorrent v4.4+.

Maybe not at the start of the stable series. Maybe at 4.4.3 and depending on how libtorrent2 is received. I intend to do the official releases based on libtorrent2.

@sledgehammer999
Copy link
Member

Nitpick but I think the commit title should be amended. This PR doesn't include anything about 2.0.4.

@sledgehammer999 sledgehammer999 merged commit 6e59248 into qbittorrent:master Jun 30, 2021
@FranciscoPombal FranciscoPombal deleted the bump_libtorrent branch July 1, 2021 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build system Issue with the build configuration or scripts (but not the code itself) CI Issues/PRs related to CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants