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

Code clean ups #19803

Merged
merged 5 commits into from
Oct 29, 2023
Merged

Code clean ups #19803

merged 5 commits into from
Oct 29, 2023

Conversation

Chocobo1
Copy link
Member

  • Migrate away from deprecated functions
  • Drop unsupported keys
  • Bump Windows macro versions
  • GHA CI: don't override cmake default CXXFLAGS

@Chocobo1 Chocobo1 added Code cleanup Clean up the code while preserving the same outcome CI Issues/PRs related to CI labels Oct 27, 2023
@Chocobo1 Chocobo1 added this to the 5.0 milestone Oct 27, 2023
@Chocobo1 Chocobo1 requested a review from a team October 27, 2023 16:37
@xavier2k6
Copy link
Member

Can you also change IsWindows7OrGreater -> IsWindows10OrGreater & remove 32 bit?

Is "Mac OS X" correct OS_TYPE?

#ifdef Q_OS_MACOS
const QString OS_TYPE = u"Mac OS X"_s;
#elif defined(Q_OS_WIN)
const QString OS_TYPE = (::IsWindows7OrGreater() && QSysInfo::currentCpuArchitecture().endsWith(u"64"))
? u"Windows x64"_s
: u"Windows"_s;
#endif


Change IsWindows8OrGreater -> IsWindows10OrGreater or remove this requirement altogether along with removing 32 bit links & only offer 3.10 64bit link?!

#ifdef QBT_APP_64BIT
const auto installerURL = ::IsWindows8OrGreater()
? u"https://www.python.org/ftp/python/3.10.11/python-3.10.11-amd64.exe"_s
: u"https://www.python.org/ftp/python/3.8.10/python-3.8.10-amd64.exe"_s;
#else
const auto installerURL = ::IsWindows8OrGreater()
? u"https://www.python.org/ftp/python/3.10.11/python-3.10.11.exe"_s
: u"https://www.python.org/ftp/python/3.8.10/python-3.8.10.exe"_s;
#endif

@xavier2k6
Copy link
Member

Is the _WIN32_IE macro really required now?

@Chocobo1
Copy link
Member Author

Chocobo1 commented Oct 28, 2023

Is the _WIN32_IE macro really required now?

I'm not sure but in the past those macro usage were intermixed & messy. You cannot leave one out otherwise weird things happens.
Speaking of which, this blog post suggests to use NTDDI_VERSION solely and I reckon we should follow but I would like to do it in a dedicated follow up PR to allow more testing about it.

@Chocobo1
Copy link
Member Author

Chocobo1 commented Oct 28, 2023

Can you also change IsWindows7OrGreater -> IsWindows10OrGreater & remove 32 bit?
Change IsWindows8OrGreater -> IsWindows10OrGreater or remove this requirement altogether along with removing 32 bit links & only offer 3.10 64bit link?!

Done.
I haven't drop code for 32 bit support. Did qbt announced no support for 32 bit? I wasn't aware.

Is "Mac OS X" correct OS_TYPE?

Not really but it is in sync with the RSS xml on fosshub. Those two must be changed together and only @sledgehammer999 can do that.

@glassez
Copy link
Member

glassez commented Oct 28, 2023

Did qbt announced no support for 32 bit? I wasn't aware.

32-bit support is gone with Qt5, isn't it?

@Chocobo1
Copy link
Member Author

32-bit support is gone with Qt5, isn't it?

Doesn't seems so. Generic Linux x86 and x86_64
https://doc.qt.io/qt-5/supported-platforms.html

It seems Qt6 has less 32-bit support. But I would defer adding more commits to this PR. I'll drop QBT_APP_64BIT later.

@glassez
Copy link
Member

glassez commented Oct 28, 2023

32-bit support is gone with Qt5, isn't it?

Doesn't seems so. Generic Linux x86 and x86_64 https://doc.qt.io/qt-5/supported-platforms.html

So why do you refer to Qt5 supported platforms. Qt6 has no support for any 32-bit desktop platform:
https://doc.qt.io/qt-6.5/supported-platforms.html

@xavier2k6
Copy link
Member

32-bit support is gone with Qt5, isn't it?

Doesn't seems so. Generic Linux x86 and x86_64 https://doc.qt.io/qt-5/supported-platforms.html

So why do you refer to Qt5 supported platforms. Qt6 has no support for any 32-bit desktop platform: https://doc.qt.io/qt-6.5/supported-platforms.html

Windows 11 is 64 bit only & Windows 10 went 64bit only from Version 2004/20H1 (granted for OEM ie new pc's) released May 27, 2020.

The official installers are x64.

@Chocobo1
Copy link
Member Author

32-bit support is gone with Qt5, isn't it?

Doesn't seems so. Generic Linux x86 and x86_64 https://doc.qt.io/qt-5/supported-platforms.html

So why do you refer to Qt5 supported platforms.

You were talking about Qt5, weren't you?

Qt6 has no support for any 32-bit desktop platform:
https://doc.qt.io/qt-6.5/supported-platforms.html

Windows 11 is 64 bit only & Windows 10 went 64bit only from Version 2004/20H1 (granted for OEM ie new pc's) released May 27, 2020.

OK but as I've said I'll deal with it in another PR.

@glassez
Copy link
Member

glassez commented Oct 28, 2023

You were talking about Qt5, weren't you?

Yes, but the context was different: since we no longer support Qt5, but only Qt6, which does not support 32-bit, then we, respectively, also do not support 32-bit.

@xavier2k6
Copy link
Member

Is the _WIN32_IE macro really required now?

I'm not sure but in the past those macro usage were intermixed & messy. You cannot leave one out otherwise weird things happens.

IE11 is EOL

Microsoft will disable Internet Explorer for good on Windows 10 version 20H2 and newer

https://www.thurrott.com/windows/279306/microsoft-is-disabling-internet-explorer-on-windows-10-today
https://www.tenforums.com/windows-10-news/202576-internet-explorer-11-now-permanently-disabled-windows-10-a.html

So perhaps we could drop the _WIN32_IE macro & raise NTDDI_VERSION to a newer OS.
This can probably be done anyway in separate PR.

https://learn.microsoft.com/en-us/deployedge/edge-ie-disable-ie11#prerequisites


Speaking of which, this blog post suggests to use NTDDI_VERSION solely and I reckon we should follow but I would like to do it in a dedicated follow up PR to allow more testing about it.

blog post is outdated, See below:

If you define NTDDI_VERSION, you must also define _WIN32_WINNT.

https://learn.microsoft.com/en-gb/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#macros-for-conditional-declarations

@Chocobo1
Copy link
Member Author

So perhaps we could drop the _WIN32_IE macro

I'm concerned about this part: (https://devblogs.microsoft.com/oldnewthing/20070411-00/?p=27283)

For example, Internet Explorer 4 came not only with an updated comctl32.dll but also a new shell32.dll ...

It means that _WIN32_IE is more than just Internet Explorer. It is somewhat related to the dlls and their shipped functions which is in use in qbt.
I'm really hesitant to drop it (as it might bring unforeseen weird behavior) so maybe you would like to handle this and submit a PR about it.

If you define NTDDI_VERSION, you must also define _WIN32_WINNT.
https://learn.microsoft.com/en-gb/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#macros-for-conditional-declarations

OK. We'll keep them.

@Chocobo1 Chocobo1 merged commit 9b924c1 into qbittorrent:master Oct 29, 2023
13 checks passed
@Chocobo1 Chocobo1 deleted the obsolete branch October 29, 2023 09:40
@xavier2k6
Copy link
Member

  • It is not necessary to specify the value of _WIN32_IE
    It is automatically set to the correct value in the sdkddkver.h Windows header based on the version set in _WIN32_WINNT.

#13327 (comment)

[REACTOS] Remove forced _WIN32_IE defines - Mar 30, 2020

No impact, as already deactivated or superseded or superfluous.

Import: https://source.winehq.org/git/wine.git/commit/7770e26f2d773dcade6f073585b6fac2661e1eb3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Issues/PRs related to CI Code cleanup Clean up the code while preserving the same outcome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants