Skip to content

Raise minimum libtorrent version to 1.2.12 (2.0.2)#14189

Merged
glassez merged 2 commits into
qbittorrent:masterfrom
glassez:libtorrent-1.2.12
Mar 1, 2021
Merged

Raise minimum libtorrent version to 1.2.12 (2.0.2)#14189
glassez merged 2 commits into
qbittorrent:masterfrom
glassez:libtorrent-1.2.12

Conversation

@glassez
Copy link
Copy Markdown
Member

@glassez glassez commented Jan 9, 2021

@glassez glassez added Core Code cleanup Clean up the code while preserving the same outcome Build system Issue with the build configuration or scripts (but not the code itself) CI Issues/PRs related to CI labels Jan 9, 2021
@glassez glassez added this to the 4.3.4 milestone Jan 9, 2021
@sledgehammer999
Copy link
Copy Markdown
Member

And why should the minimum be raised?

@glassez
Copy link
Copy Markdown
Member Author

glassez commented Jan 21, 2021

And why should the minimum be raised?

Apparently, this was decided as a result of another discussion that you missed during your long absence...
The bottom line is that we recognized the previously existing practice of supporting multiple versions of libtorrent (and therefore conditional support for some features) as undesirable.
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.
The second negative point concerns the web API. Having conditional inclusion of some functions makes it unstable in the sense that a specific version of the API does not guarantee the presence of these functions, which makes versioning meaningless.
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).
Do you have any serious objections?

@sledgehammer999
Copy link
Copy Markdown
Member

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

As long as this holds true I don't have a problem. Your explanation satisfies my curiosity.

@ghost
Copy link
Copy Markdown

ghost commented Jan 22, 2021

Windows vcpkg CI failed due to having older libtorrent.

@glassez
Copy link
Copy Markdown
Member Author

glassez commented Jan 22, 2021

Windows vcpkg CI failed due to having older libtorrent.

Yes, vcpkg is still lagging behind: microsoft/vcpkg#15620. This is what prevents this PR from being merged.

Comment thread src/base/bittorrent/session.h
@jagannatharjun
Copy link
Copy Markdown
Member

PS: we should move away from vcpkg.

@glassez
Copy link
Copy Markdown
Member Author

glassez commented Jan 23, 2021

we should move away from vcpkg.

Right now, it's only used for CI. @FranciscoPombal promoted vcpkg by motivating it with the convenience of automating the build. But the other side of the coin is a significant slowdown in access to new libtorrent versions. In case of this PR, this is not critical, but it may be different if the new version contains important fixes.
If we are going to continue with vcpkg we need to at least maintain the libtorrent port ourselves.
Otherwise we need to setup vcpkg-free CI.

@sledgehammer999
Copy link
Copy Markdown
Member

If Actions CI for linux supports pulling packages from PPA, then I can make a PPA that will autopull+autobuild from libtorrent's git every commit on the branches we care.

@glassez
Copy link
Copy Markdown
Member Author

glassez commented Jan 24, 2021

If Actions CI for linux...

The problem is Actions CI for Windows/MacOS where vcpkg is used.

@jagannatharjun
Copy link
Copy Markdown
Member

jagannatharjun commented Jan 24, 2021

Otherwise we need to setup vcpkg-free CI.

one advantage of vcpkg is that it makes static builds quite easy. A better approach would be to only build libtorrent ourselves. it shouldn't be that hard.

@glassez glassez force-pushed the libtorrent-1.2.12 branch 5 times, most recently from 807aeba to a9404f6 Compare January 24, 2021 17:32
@glassez
Copy link
Copy Markdown
Member Author

glassez commented Jan 24, 2021

I am trying to use a custom libtorrent vcpkg port, but so far without success... Any ideas?

@jagannatharjun
Copy link
Copy Markdown
Member

I am trying to use a custom libtorrent vcpkg port, but so far without success... Any ideas?

probably actions are using cached vcpkg rather than building libtorrent. the actual setup of vcpkg with building qt and libtorrent would take well over 1 hour.

@glassez glassez force-pushed the libtorrent-1.2.12 branch 2 times, most recently from 4c11d06 to 1fc52e6 Compare January 25, 2021 06:25
@glassez
Copy link
Copy Markdown
Member Author

glassez commented Jan 25, 2021

I am trying to use a custom libtorrent vcpkg port, but so far without success... Any ideas?

probably actions are using cached vcpkg rather than building libtorrent. the actual setup of vcpkg with building qt and libtorrent would take well over 1 hour.

It looks like we need to call vcpkg upgrade to update previously installed packages (vcpkg install does not do this).
Now Windows successfully uses a custom port. But the problem is with MacOS. It fails to build libtorrent-1.2.12 for some reason.

@glassez
Copy link
Copy Markdown
Member Author

glassez commented Jan 26, 2021

Finally, I managed to get it compiled in GitHub CI.
Appveyor is still an outsider. Unfortunately, it is under responsibility of a single person. It would be nice if it did not require manual loading of packages compiled outside of it, but acted in the same way as other our CI environments (automatically downloaded packages from public sources or compiled them itself).

Otherwise we can get rid of it altogether.

@sledgehammer999?

@glassez glassez requested review from a team, Chocobo1 and sledgehammer999 January 26, 2021 14:49
Comment thread .github/workflows/ci.yaml Outdated
Comment thread vcpkg/libtorrent/CONTROL Outdated
Comment thread vcpkg/libtorrent/CONTROL Outdated
@glassez glassez force-pushed the libtorrent-1.2.12 branch 3 times, most recently from 46fb128 to 4374db7 Compare January 28, 2021 08:23
Chocobo1
Chocobo1 previously approved these changes Jan 28, 2021
Chocobo1
Chocobo1 previously approved these changes Jan 28, 2021
@glassez glassez force-pushed the libtorrent-1.2.12 branch 4 times, most recently from 53e241b to b673e0c Compare January 29, 2021 17:19
@glassez glassez requested a review from Chocobo1 January 29, 2021 17:21
@sledgehammer999
Copy link
Copy Markdown
Member

Finally, I managed to get it compiled in GitHub CI.
Appveyor is still an outsider. Unfortunately, it is under responsibility of a single person. It would be nice if it did not require manual loading of packages compiled outside of it, but acted in the same way as other our CI environments (automatically downloaded packages from public sources or compiled them itself).

Otherwise we can get rid of it altogether.

@sledgehammer999?

I can update the dependencies if you want. Or the AppVeyor CI can be dropped if it is covered by another CI. It seems that Actions CI tests on Windows too. Does it cover the same things as the AppVeyor configuration? If yes, then AppVeyor can be dropped IMO.

@sledgehammer999
Copy link
Copy Markdown
Member

it is under responsibility of a single person.

If AppVeyor can't be dropped, then I think it is possible to give access to other persons to upload necessary packages to builds.shiki.hu/appveyor/

@glassez
Copy link
Copy Markdown
Member Author

glassez commented Feb 6, 2021

It seems that Actions CI tests on Windows too.

It builds using CMake but it seems we still need to test QMake too. IMO, we can drop Appveyor if someone provides Windows+qmake configuration for GitHub Actions CI.

If AppVeyor can't be dropped, then I think it is possible to give access to other persons to upload necessary packages to builds.shiki.hu/appveyor/

You can do that 👍

@sledgehammer999
Copy link
Copy Markdown
Member

I updated AppVeyor and the build has passed.

@glassez glassez merged commit a2b0531 into qbittorrent:master Mar 1, 2021
@glassez glassez deleted the libtorrent-1.2.12 branch March 1, 2021 17:43
Comment thread .github/workflows/ci.yaml
Add-Content ${{ github.workspace }}/triplets_overlay/x64-osx-release.cmake `
-Value "set(VCPKG_BUILD_TYPE release)","set(VCPKG_OSX_DEPLOYMENT_TARGET 10.15)"

# NOTE: Avoids a libtorrent ABI issue. See https://github.com/arvidn/libtorrent/issues/4965
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glassez You know that I'm back now, but you couldn't at least ping me before merging this, or at least included a link to this in here: https://github.com/orgs/qbittorrent/teams/bug-handlers/discussions/13/comments/4? I would have liked to review this...

Case in point, I don't think this change is good. The workaround you found was to always force passing -DCMAKE_CXX_STANDARD=17 to everything, but this is not a good practice in CMake. It should only be done if needed, and thus should remain exclusive to the macOS build.

Another disadvantage is that it makes our portfile deviate more from upstream than necessary. We don't want to deviate from upstream too much. In fact, it is in our best interest to contribute back, so that we may benefit from the efforts of others and integrate upstream fixes more easily. It's not like we are hyper-specialized users of libtorrent.

Feel free to submit a hotfix. if you don't have the time for that, I'll get round to it once I've finished catching up with current happenings, or maybe someone else can do it in the meantime.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it should have been merged a long time ago. I just forgot about it.

Case in point, I don't think this change is good. The workaround you found was to always force passing -DCMAKE_CXX_STANDARD=17 to everything, but this is not a good practice in CMake. It should only be done if needed, and thus should remain exclusive to the macOS build.

I agree. But I didn't want to be perfect at it. I just wanted to avoid an obstacle in the form of vcpkg, so that CI would continue to work.

if you don't have the time for that, I'll get round to it once I've finished catching up with current happenings

I'll leave it to you.

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 Code cleanup Clean up the code while preserving the same outcome Core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants