Fix wrong version conditional

[Chocobo1]

• Fix wrong version conditional
  torrent_file_with_hashes() was introduced in libtorrent v2.0.3.
  https://github.com/arvidn/libtorrent/releases/tag/v2.0.3
• Don't use old style cast
  Moved to Detect incompatible libtorrent v2 versions in build systems  #15242.

[glassez]

• Fix wrong version conditional
  torrent_file_with_hashes() was introduced in libtorrent v2.0.3.
  https://github.com/arvidn/libtorrent/releases/tag/v2.0.3

It was done intentionally. We don't care about unsupported versions of libtorrent. So we can easily use only one condition for separating libtorrent-1.2 and libtorrent-2.0 branches. Although it would make sense not to use exact libtorrent version directly but provide special constant for this purpose, e.g. #if QBT_USES_LIBTORRENT2 to prevent such misinterpretation like you've done here.

[FranciscoPombal]

It was done intentionally. We don't care about unsupported versions of libtorrent. So we can easily use only one condition for separating libtorrent-1.2 and libtorrent-2.0 branches. Although it would make sense not to use exact libtorrent version directly but provide special constant for this purpose, e.g. #if QBT_USES_LIBTORRENT2 to prevent such misinterpretation like you've done here.

Previous discussion/context for this: #15093 (comment), #15093 (comment)

[Chocobo1]

It was done intentionally. We don't care about unsupported versions of libtorrent. So we can easily use only one condition for separating libtorrent-1.2 and libtorrent-2.0 branches.

This "intention" would only be valid if there are correct detection for libtorrent v2 versions. Currently there are none.
Furthermore, it is wrong at the very root to use a wrong version number to represent such "intention".

Although it would make sense not to use exact libtorrent version directly but provide special constant for this purpose, e.g. #if QBT_USES_LIBTORRENT2 to prevent such misinterpretation like you've done here.

I don't mind having that macro but before having that I intend to fix the compile error at my hand.

[FranciscoPombal]

@Chocobo1

I don't mind having that macro but before having that I intend to fix the compilation error at my hand.

You'd only get a compilation error if you're compiling with libtorrent < 2.0.3, which we explicitly don't support (the minimum 2.0.x version is now 2.0.4), so that is a non-issue.

[glassez]

Although it would make sense not to use exact libtorrent version directly but provide special constant for this purpose, e.g. #if QBT_USES_LIBTORRENT2 to prevent such misinterpretation like you've done here.

I don't mind having that macro but before having that I intend to fix the compile error at my hand.

Don't forget that we don't REALLY care about unsupported libtorrent versions, so if you intend to use one of them, then you risk getting a logically incorrect application, even if it compiles successfully. So I would prefer to direct efforts to ensure that the build scripts correctly handle the minimum supported version of both libtorrent branches.

[Chocobo1]

Don't forget that we don't REALLY care about unsupported libtorrent versions, so if you intend to use one of them, then you risk getting a logically incorrect application, even if it compiles successfully.

This is really a gray area: neither INSTALL, cmake scripts, autotools script mentioned anything about the lower bound of libtorrent 2.x.

So I would prefer to direct efforts to ensure that the build scripts correctly handle the minimum supported version of both libtorrent branches.

This is indeed an issue to fix. And since you suggested the new QBT_USES_LIBTORRENT2 macro I'll let you implement it. But until that happens I still intend to fix this wrong version number issue. IMO the commit is valid and it doesn't justify to block it in order to wait for another marco to be implemented (unless you could do it quickly so this PR commit would be obsolete).

Also to mention, another solution is to correctly specify all the version conditionals such that the code would be correct even without the build system detection.

[FranciscoPombal]

@Chocobo1

This is really a gray area: neither INSTALL, cmake scripts, autotools script mentioned anything about the upper bound of libtorrent < 2.0.

Usage of libtorrent 2.0.x is considered an opt-in "experimental" feature that can be used if desired. This facilitates migration to 2.0.x and testing of it while we keep releasing with 1.2.x and recommending it to the majority of users.

Whether you choose to compile with 1.2.x or 2.0.x, there is a lower bound of the latest stable point release for both branches. In the past, both me and @glassez have submitted PRs indicating these requirements (the 2.0.x requirement is mentioned in parens): #14673, #15093. Currently, the minimum requirements for building qBittorrent are libtorrent 1.2.14 or 2.0.4.

This is indeed an issue to fix. And since you suggested the new QBT_USES_LIBTORRENT2 macro I'll let you implement it. But until that happens I still intend to fix this wrong version number issue. IMO the commit is valid and it doesn't justify to block it in order to wait for another marco to be implemented (unless you could do it quickly so this PR commit would be obsolete).

Just don't touch any of the conditionals until such a macro is implemented then.
As I've said before, there is no "compilation error". If building with 2.0.x, you must use >= 2.0.4.
Again, we've been over this in #15093 (comment), #15093 (comment) (unless @glassez's opinion has changed).

[glassez]

IMO the commit is valid

This commit will only lead to the fact that it will create a false impression that qBittorrent supports actually incompatible versions of libtorrent2 (i.e. <= 2.0.2). Why do you need it?

unless you could do it quickly

Well, I will try to do something in the next couple of days, although I do not believe that preventing these regressive changes (according to previously made decisions) requires this.

[Chocobo1]

This commit will only lead to the fact that it will create a false impression that qBittorrent supports actually incompatible versions of libtorrent2 (i.e. <= 2.0.2). Why do you need it?

Well, I will try to do something in the next couple of days, although I do not believe that preventing these regressive changes (according to previously made decisions) requires this.

I think I start understanding your viewpoint, let me explain mine.
There are 2 things missing/wrong in the current state and both should be fixed (separately):

1. Missing version checks for libtorrent < 2.0.4
   As implied before, if you want to enforce version requirements it should be done in INSTALL file, cmake and autotools scripts.
2. Wrong conditional in c++ code
   Since version checking/enforcing is done in build systems, the c++ code should only be concern of being correct without any other intention behind it. Correct as in being truthful with the correct version numbers especially here is comparing version numbers.
   The first commit does restore some compatibility with previous lib versions but that is not the point of the change, it is fixing inconsistency of the code.

The way you explain, it sounds like you are trying to enforce version requirements at c++ level. This concept itself is not wrong but it will become absurd when there are established build systems at hand. Furthermore, if you want to enforce it at c++ level, then you need more mechanisms than just detecting LIBTORRENT_VERSION_NUM, such as emitting compile errors and error messages. IMO deliberately breaking compilation in the wrong way and having inconsistent code isn't justified in any case.

[glassez]

I do not deny the shortcomings in the handling of compatible versions of libtorrent2. Apparently, no one (including me) wanted to spend time on this while libtorrent2 support was in an experimental state, so we were content with only an in-project declaration of the minimally supported version of libtorrent2.
Now that lt2 is selected for use in v4.4, it would be better and more convenient to completely remove libtorrent-1.2 support from the master and leave it only in v4_3_x branch. But for some reason, @sledgehammer999 wants to keep it for now. So we still have to go a more difficult way.

[FranciscoPombal]

@Chocobo1

@glassez's previous comment encompasses the essence of the problem. These are just temporary growing pains of the transition while we support both branches.

If you are really bothered by this, I could add a check in CMake (e.g., forbid version greater than 1.2.x unless some hidden flag such as EXPERIMENTAL_LT2_SUPPORT is ON) and document the dual requirement in the INSTALL file (or you could do it in this PR if you wish), instead of altering the conditionals. Let me know what you decide. EDIT: seems I'm a little outdated and #15242 was already posted.

[Chocobo1]

Superseded by PR #15297.