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

WTF This piece of cmake code is not ported to Qt 5 #24385

Closed
h0tc0d3 opened this issue Apr 19, 2022 · 31 comments
Closed

WTF This piece of cmake code is not ported to Qt 5 #24385

h0tc0d3 opened this issue Apr 19, 2022 · 31 comments
Labels

Comments

@h0tc0d3
Copy link

h0tc0d3 commented Apr 19, 2022

Please fix error and replace range v3 code with stdlib #24014

[ 53%] Building CXX object Telegram/CMakeFiles/Telegram.dir/SourceFiles/api/api_hash.cpp.o
[ 53%] Building CXX object Telegram/CMakeFiles/Telegram.dir/SourceFiles/api/api_polls.cpp.o
[ 53%] Building CXX object Telegram/CMakeFiles/Telegram.dir/SourceFiles/api/api_sending.cpp.o
In file included from <built-in>:485:
In file included from /build/telegram-desktop/src/build/Telegram/CMakeFiles/Telegram.dir/cmake_pch.hxx:5:
In file included from /build/telegram-desktop/src/tdesktop-3.5.0-full/Telegram/SourceFiles/stdafx.h:102:
In file included from /usr/include/range/v3/all.hpp:17:
In file included from /usr/include/range/v3/action.hpp:32:
In file included from /usr/include/range/v3/action/split.hpp:28:
/usr/include/range/v3/range/conversion.hpp:379:29: error: alias template 'QVector' requires template arguments; argument deduction only allowed for class templates
                -> decltype(ContT(range_cpp17_iterator_t<Rng>{},
                            ^
/usr/include/meta/meta.hpp:541:5: note: in instantiation of template class 'ranges::detail::from_range<QVector>' requested here
    using invoke = typename Fn::template invoke<Args...>;
    ^
/usr/include/range/v3/range/conversion.hpp:46:13: note: in instantiation of template type alias 'invoke' requested here
            using container_t = meta::invoke<MetaFn, Rng>;
            ^
/usr/include/range/v3/range/conversion.hpp:338:46: note: in instantiation of template type alias 'container_t' requested here
                    convertible_to_cont<Rng, container_t<MetaFn, Rng>>)
                                             ^
/usr/include/range/v3/detail/prologue.hpp:35:26: note: expanded from macro 'template'
    template<__VA_ARGS__ CPP_TEMPLATE_AUX_                                                               ^
/usr/include/range/v3/range/conversion.hpp:304:13: note: in instantiation of template class 'ranges::detail::to_container::fn<ranges::detail::from_range<QVector>>' requested here
          , Fn
            ^
/usr/include/range/v3/range/conversion.hpp:425:20: note: in instantiation of template class 'ranges::detail::to_container::closure<ranges::detail::from_range<QVector>, ranges::detail::to_container::fn<ranges::detail::from_range<QVector>>>' requested here
            return detail::to_container_fn<detail::from_range<ContT>>{}(
                   ^
/build/telegram-desktop/src/tdesktop-3.5.0-full/Telegram/SourceFiles/api/api_media.cpp:86:12: note: in instantiation of function template specialization 'ranges::_to_::to<QVector, std::vector<tl::boxed<MTPinputDocument>> &>' requested here
                        ranges::to<QVector>(info.attachedStickers)),
                                ^
/usr/include/qt6/QtCore/qcontainerfwd.h:63:22: note: template is declared here
template<typename T> using QVector = QList<T>;
                     ^
In file included from <built-in>:485:
In file included from /build/telegram-desktop/src/build/Telegram/CMakeFiles/Telegram.dir/cmake_pch.hxx:5:
In file included from /build/telegram-desktop/src/tdesktop-3.5.0-full/Telegram/SourceFiles/stdafx.h:102:
In file included from /usr/include/range/v3/all.hpp:17:
In file included from /usr/include/range/v3/action.hpp:32:
In file included from /usr/include/range/v3/action/split.hpp:28:
/usr/include/range/v3/range/conversion.hpp:425:20: error: type 'detail::to_container_fn<detail::from_range<QVector>>' (aka 'closure<ranges::detail::from_range<QVector>, ranges::detail::to_container::fn<ranges::detail::from_range<QVector>>>') does not provide a call operator
            return detail::to_container_fn<detail::from_range<ContT>>{}(
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/telegram-desktop/src/tdesktop-3.5.0-full/Telegram/SourceFiles/api/api_media.cpp:86:12: note: in instantiation of function template specialization 'ranges::_to_::to<QVector, std::vector<tl::boxed<MTPinputDocument>> &>' requested here
                        ranges::to<QVector>(info.attachedStickers)),
                                ^
2 errors generated.
make[2]: *** [Telegram/CMakeFiles/Telegram.dir/build.make:493: Telegram/CMakeFiles/Telegram.dir/SourceFiles/api/api_media.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/Makefile2:1550: Telegram/CMakeFiles/Telegram.dir/all] Error 2
make: *** [Makefile:136: all] Error 2
==> ERROR: A failure occurred in build().
    Aborting...
==> ERROR: Build failed, check /build
@h0tc0d3 h0tc0d3 added the bug label Apr 19, 2022
@ilya-fedin
Copy link
Contributor

ilya-fedin commented Apr 19, 2022

Qt 5 is no more supported on Linux. If you need that, you can port that code on your own. This is not the only thing you would need to port to Qt 5 in 3.7, though.

@ilya-fedin
Copy link
Contributor

As for #24014, you need to either build with gcc or wait until clang would add the required C++20 feature

@h0tc0d3
Copy link
Author

h0tc0d3 commented Apr 19, 2022

@ilya-fedin you must understand that this is a regression and this is not correct!
You should not use experimental functions in large projects and break the compilation for saving a couple of lines of code. If it's a C++20 standard, then it must be hard-coded into CXXFLAGS, but that's nowhere to be found.

grep -liR "c++20" .
./Telegram/ThirdParty/libtgvoip/lib_tgvoip.cmake
./Telegram/ThirdParty/libtgvoip/webrtc_dsp/absl/meta/type_traits.h
./Telegram/ThirdParty/range-v3/.github/workflows/range-v3-ci.yml
./Telegram/ThirdParty/range-v3/README.md
./Telegram/ThirdParty/range-v3/cmake/ranges_flags.cmake
./Telegram/ThirdParty/range-v3/cmake/ranges_options.cmake
./Telegram/ThirdParty/range-v3/doc/index.md
./Telegram/ThirdParty/range-v3/doc/release_notes.md
./Telegram/ThirdParty/range-v3/include/concepts/compare.hpp
./Telegram/ThirdParty/range-v3/include/concepts/concepts.hpp
./Telegram/ThirdParty/range-v3/include/std/detail/associated_types.hpp
./Telegram/ThirdParty/xxHash/.github/workflows/ci.yml
./cmake/options_linux.cmake

Also, if clang does not include some functionality of the C ++ 20 standard, then there is an important reason for this, at least speed degradation. Also some C/C++ features included in GCC are against other standards like IEEE etc and some functions are marked experimental.

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Apr 19, 2022

This is not an experimental feature and C++20 is specified as the minimum required standard
https://github.com/desktop-app/cmake_helpers/blob/master/init_target.cmake#L7

@ilya-fedin
Copy link
Contributor

Also, if clang does not include some functionality of the C ++ 20 standard, then there is an important reason for this, at least speed degradation.

If clang doesn't introduce something, it's clang's problem, all officially supported configurations are building fine, everything else could be fixed by PRs from people who is interested in such configurations.

@h0tc0d3
Copy link
Author

h0tc0d3 commented Apr 19, 2022

@ilya-fedin

set(MAXIMUM_CXX_STANDARD cxx_std_20)

cmake not contains property MAXIMUM_CXX_STANDARD https://cmake.org/cmake/help/latest/search.html?q=MAXIMUM_CXX_STANDARD

Your search did not match any documents. Please make sure that all words are spelled correctly and that you've selected enough categories.

MAXIMUM_CXX_STANDARD only in https://github.com/desktop-app/cmake_helpers/blob/master/init_target.cmake#L7

It's your problem as a developer, you shouldn't do regressions. This is a regression.
clang is the smartest compiler and best error control. It is foolish not to take it into account, and take into account more of the curve gcc.

@ilya-fedin
Copy link
Contributor

MAXIMUM_CXX_STANDARD only in

grep the file...

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Apr 19, 2022

It's your problem as a developer, you shouldn't do regressions. This is a regression.

You couldn't really require something on github. Either you PR or shut up and wait until someone else would take time and interest and solve your problem.

@h0tc0d3
Copy link
Author

h0tc0d3 commented Apr 19, 2022

MAXIMUM_CXX_STANDARD only in

grep the file...

rep -lIR "MAXIMUM_CXX_STANDARD" .
./cmake/init_target.cmake

@ilya-fedin
Copy link
Contributor

Grep the file itself lol

@h0tc0d3
Copy link
Author

h0tc0d3 commented Apr 19, 2022

@ilya-fedin So, good error dolution!

You couldn't really require something on github. Either you PR or shut up and wait until someone else would take time and interest and solve your problem.

So, this is good error solution!
You must understand once and for all that regression is bad!
If everyone wrote code so mindlessly, then for example, the Linux kernel and other larger projects would be a mess!

@ilya-fedin
Copy link
Contributor

It's your problem as a developer, you shouldn't do regressions. This is a regression.

Also, I don't really agree with that matter. If something is not (first-class) supported, it's clear it will regress. And it's the job of the one who is interested in supporting this to fix this.

@h0tc0d3
Copy link
Author

h0tc0d3 commented Apr 19, 2022

Also, I don't really agree with that matter. If something is not (first-class) supported, it's clear it will regress. And it's the job of the one who is interested in supporting this to fix this.

You didn't just break the code in one place, you broke the code in two places.

@ilya-fedin
Copy link
Contributor

And?

@ilya-fedin
Copy link
Contributor

None of the officially supported configurations regressed. Everything else is community-supported and it's community's job to fix these configurations.

@h0tc0d3
Copy link
Author

h0tc0d3 commented Apr 19, 2022

@ilya-fedin this is a complete regression.You have broken the build capability.

@ilya-fedin
Copy link
Contributor

ilya-fedin commented Apr 19, 2022

If you don't really support something, you don't regress on that (as it's not supported in the first place).

@h0tc0d3
Copy link
Author

h0tc0d3 commented Apr 19, 2022

@ilya-fedin I commit to a lot of projects and the changes just won't be accepted into any major projects if there is a regression.

@ilya-fedin
Copy link
Contributor

If it's a regression to something supported then yeah. If it regresses in something that is not really supported and no one checks such configurations then no one would care about that.

@ilya-fedin
Copy link
Contributor

You just surprised that clang to tdesktop as niche platforms to some projects. Now you know. And I hope you will stop require anyone do anything on github, no one works for you here and no one will do anything just because you think that's how it should be.

@h0tc0d3
Copy link
Author

h0tc0d3 commented Apr 19, 2022

you will stop require anyone do anything on github

I do not demand anything, just normal developers usually try to solve such problems, and not shift them to the community. I personally have no time to understand the tdesktop code, as I am busy with other projects, and studying the code takes time. And as far as possible, I send patches if some package stops being built by clang. If you do not have enough hands, which is in many open source projects, then you need to look for students and engage in mentoring, for example, through gsoc. For any normal project, it's considered good practice to pay attention to bug fixes.

@ilya-fedin
Copy link
Contributor

I do not demand anything

Well, it sounded like that.

just normal developers usually try to solve such problems, and not shift them to the community.

I guess you usually talk with people from community who is interested in supporting clang. I'm from community, I'm not interested in that though (right now at least).

@pg83
Copy link
Contributor

pg83 commented Apr 21, 2022

Can I ask, how you build tdesktop for darwin/macos? It is official compiler is clang.

@pg83
Copy link
Contributor

pg83 commented Apr 21, 2022

Можно я на русском? Вроде, все свои.

Я тоже заинтересован в поддержке сборки через clang. Если я это починю, расставив по месту вызов ranges::to<QVector<ConcreteType>>(...), такой патч будет принят в апстрим?

@23rd
Copy link
Collaborator

23rd commented Apr 21, 2022

Can I ask, how you build tdesktop for darwin/macos? It is official compiler is clang.

AppleClang uses Qt6.

@pg83
Copy link
Contributor

pg83 commented Apr 21, 2022

Can I ask, how you build tdesktop for darwin/macos? It is official compiler is clang.

AppleClang uses Qt6.

strange, I have same error(in another file) with qt6/linux
will check my setup

@ilya-fedin
Copy link
Contributor

It is official compiler is clang.

AppleClang seem to handle such code just fine

@pg83
Copy link
Contributor

pg83 commented Apr 21, 2022

It is official compiler is clang.

AppleClang seem to handle such code just fine

Sounds really strange, cause apple clang have same frontend that of stock clang.

@ilya-fedin
Copy link
Contributor

Sounds really strange, cause apple clang have same frontend that of stock clang.

Apparently it's not really the same

@h0tc0d3
Copy link
Author

h0tc0d3 commented Apr 22, 2022

@pg83 most likely to be accepted. But honestly, I'm already tired that after each release, the compilation breaks down and I have to spend my time fixing it. Even chromium doesn't take me that long. I don't know why they are prevented from using the latest version of clang and clang-tidy static analyzer. Could identify some of the potential errors at the development stage. There are plenty of errors in telegram, and gcc won't always point them out. At least if clang can compile, then other compilers usually can too. But this does not work for other compilers.

I want to note the problem occurs only with qt 6, with qt5 it compiles successfully. This is hardly a compiler problem, in my opinion, gcc just compiles code where clang thinks that we can get unpredictable behavior or there are ambiguous interpretations of the C++ standard.

@john-preston
Copy link
Member

@h0tc0d3 AppleClang is based not on the latest clang, I think.

There is current development flow, where MSVC is used on Windows, AppleClang on macOS and GCC on Linux. All of them have their problems and that way you get the most portable code.

This is not an error in Telegram, this is an error in clang, as pointed out by range-v3 author. It can be worked around somehow, I guess, so PRs are welcome.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants