Bug 453759 - KSyntaxHighlighting does not build with a C++20 compiler on Windows
Summary: KSyntaxHighlighting does not build with a C++20 compiler on Windows
Status: RESOLVED FIXED
Alias: None
Product: frameworks-syntax-highlighting
Classification: Frameworks and Libraries
Component: framework (show other bugs)
Version: 5.93.0
Platform: Microsoft Windows Microsoft Windows
: NOR critical
Target Milestone: ---
Assignee: KWrite Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-05-13 19:28 UTC by Nicolas Arnaud-Cormos
Modified: 2022-06-15 11:41 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.95.0
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nicolas Arnaud-Cormos 2022-05-13 19:28:13 UTC
SUMMARY
KSyntaxHighlighting does not compile with Visual Studio 2022 and C++20.
(You need a C++20 compiler that support the new format standard library feature)

STEPS TO REPRODUCE
1. Compile KSyntaxHighlighting with Visual Studio 2022 in C++20 mode...

OBSERVED RESULT
Thousands of errors like that:
```
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\chrono(4595): error C2760: syntax error: unexpected token 'identifier', expected 'type specifier'
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\chrono(4622): error C3646: '_Alignment': unknown override specifier
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\chrono(4629): note: see reference to class template instantiation 'std::_Chrono_format_specs<_CharT>' being compiled
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\chrono(4622): error C2059: syntax error: '='
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.29.30133\include\chrono(4622): error C2653: '_Fmt_align': is not a class or namespace name
```

EXPECTED RESULT
It builds ;)

ADDITIONAL INFORMATION
The problem comes from the generation of headers with ecm_generate_headers.
One of the header is "Format", which will be mixed up with the standard library one "format". Obviously, this error does not happen on Linux.
I see 2 ways to fix it:
- complex: change the generated header to something else than Format (may impact other projects)
- easy: don't call ecm_generate_headers on Windows, or at least add a way to not do it with a CMake variable

Maybe there's another one, I don't know.
Comment 1 Nicolas Fella 2022-05-14 14:07:41 UTC
case-insensitive filesystem strikes again :facepalm:

the problem isn't so much ecm_generate_headers, rather it's that we want to install a header named "Format", no?

Not doing that would be a source incompatible change
Comment 2 Nicolas Arnaud-Cormos 2022-05-14 17:56:58 UTC
(In reply to Nicolas Fella from comment #1)
> case-insensitive filesystem strikes again :facepalm:
> 
> the problem isn't so much ecm_generate_headers, rather it's that we want to
> install a header named "Format", no?
> 
> Not doing that would be a source incompatible change

Yes, the problem is the "Format" include file.
I don't see how you could fix that without a source compatible change, that's why I proposing an option for those using KSyntaxHighlighting on Windows. Between that or not using the lib, I'll go with the source compatible change ;)
Comment 3 Ahmad Samir 2022-05-29 10:32:28 UTC
After https://invent.kde.org/frameworks/syntax-highlighting/-/commit/42a061b04c507de11b0fb240808fc65a7528d946
The headers are now:
/usr/include/KF5/KSyntaxHighlighting/KSyntaxHighlighting/Format
/usr/include/KF5/KSyntaxHighlighting/KSyntaxHighlighting/format.h

does that fix the issue here? this commit is in 5.94.0.
Comment 4 Ahmad Samir 2022-05-29 11:09:57 UTC
I think how the repo's headers are included in its own source files needs to be cleaned up too.
Comment 5 Bug Janitor Service 2022-05-29 11:47:33 UTC
A possibly relevant merge request was started @ https://invent.kde.org/frameworks/syntax-highlighting/-/merge_requests/313
Comment 6 Friedrich W. H. Kossebau 2022-05-29 12:45:15 UTC
The problem is not the header named "Format", but when  include directories are set up to also have the very subdirectory of the "Format" file as part of it. Which is not by design, instead only the prefix "KSyntaxHighlighting" directoy should be found in the include directories.
So that only #include <KSyntaxHighlighting/Format> will work.

My question would be: can you tell which include actually triggers the wrong header to be used? I saw no include statement in KSyntaxHighlighting itself that uses a plain "format"/"Format" of  any kind? So curious how exactly the wrong pick by the compiler is triggered here?
Comment 7 Friedrich W. H. Kossebau 2022-05-29 14:10:44 UTC
Thanks to a shower thought I guess the problem is rather that the current buildsystem setup for KSyntaxHighlighting generates the forward camelcase headers directly in the build directory of the library, which also is part of the include directories set up for the build, and before any system directories. So the preprocessor on searching to resolve #include <format> will simply first come across the "Format" file in the library build dir, and then things go boom.

So the generated headers need to be placed outside of the include directories, or, in this case due to other planned usages like from the examples referencing the generated forward headers, be placed into a subdir matching the prefix dir as used in the installed case. 

So the part of https://invent.kde.org/frameworks/syntax-highlighting/-/merge_requests/313 which adds the "OUTPUT_DIR" argument to the ecm_generate_headers call should improve things and avoid the name collision.
Comment 8 Friedrich W. H. Kossebau 2022-06-01 13:06:20 UTC
Nicolas: I am curious: given ksyntaxhighlighting itself is set to use C++17 during its build, does MSVC fail to react to that and still make C++20 headers visible or even use them? 

Or, when you say "Compile KSyntaxHighlighting with Visual Studio 2022 in C++20 mode" you mean, you modified the normal build and actually enforce C++20 instead? So anyone else should not be really affected for now? And in that case, why would you build with C++20 and not just the default C++17? Does that affect the output in any way? And should it perhaps be an option supported by upstream (in case this is also interesting for others)?
Comment 9 Nicolas Arnaud-Cormos 2022-06-01 13:14:43 UTC
Friedrich: Sorry, I wasn't clear, this is when integrating KSyntaxHighlighting with another application - this one using C++20.
I did the integration with CMake/FetchContent.

At this point, you have 2 different headers competing with the same name, as the KSyntaxHighlighting path is added to the include path.
I think the patch from Ahmad could work, as you must now prepentd KSyntaxHighlighting to the include, but I didn't have a chance to test yet.
Comment 10 Friedrich W. H. Kossebau 2022-06-01 13:31:24 UTC
So a work-around for you might be to set the CMake C++  standard  explicitly to C++17 for that subdirectory with ksyntaxhighligting, to match what is officially supported. Building the code with C++20 only works by chance, so there might be future issues.

)s your code open visible somewhere, to get a better idea of your use-case?

Still we want to fix ksyntaxhighlighting from the regression to have the unprefixed headers in the build include dir, that is not that ideal in any case, just wondering how urgent it is.
Comment 11 Nicolas Arnaud-Cormos 2022-06-01 14:47:25 UTC
I don't see how it will help me.

The problem is not necessary when building KSyntaxHighlighting (even if in my case I'm also building it with C++20), but rather when using it in another library/application, with the include paths containing the direct path to the Format header.
Which is the case for me when using fetchcontent from CMake.

The patch from Ahmad should help, as now you need to #include <KSyntaxHighlighting/Format> to access it, so #include <format> should point to the right one in the std.
Comment 12 Friedrich W. H. Kossebau 2022-06-01 14:53:31 UTC
Ah, right, yes, I only thought about building KSyntaxHighlighting itself, as the bug report talks about in title and initial description.

 Once it itself is built (if that works) and you build against the build artifacts in its build dir, we are back at the problem, I see,
Comment 13 Bug Janitor Service 2022-06-01 19:52:51 UTC
A possibly relevant merge request was started @ https://invent.kde.org/frameworks/syntax-highlighting/-/merge_requests/314
Comment 14 Friedrich W. H. Kossebau 2022-06-03 09:57:22 UTC
Git commit 8664e6eb1d3b45805abffef91fe212d253750bc0 by Friedrich W. H. Kossebau.
Committed on 01/06/2022 at 19:53.
Pushed by kossebau into branch 'master'.

Avoid unprefixed CamelCase headers generated directly in build dir

The removal of the PREFIX argument from ecm_generate_headers() call
in 42a061b04c507de11b0fb240808fc65a7528d946 resulted in the headers
being directly generated in the build dir, thus no longer only being
visible via an explicit prefixed <KSyntaxHighlighting/*> include.
FIXED-IN: 5.95.0

M  +6    -0    src/lib/CMakeLists.txt

https://invent.kde.org/frameworks/syntax-highlighting/commit/8664e6eb1d3b45805abffef91fe212d253750bc0
Comment 15 Ahmad Samir 2022-06-15 11:41:10 UTC
Git commit c070f3338da63feb0f81170608f9c5de76f98e3c by Ahmad Samir.
Committed on 14/06/2022 at 12:00.
Pushed by ahmadsamir into branch 'master'.

Adjust repo's own includes

Use ForwardingHeaders for public headers.

Also only explicitly add the ForwardingHeader dir added to the target
include dirs; because KDECMakeSettings from ECM already sets
CMAKE_INCLUDE_CURRENT_DIR_IN_INTERFACE, so no need to duplicate that
thanks, to Friedrich W. H. Kossebau for pointing that out.

M  +5    -5    autotests/foldingtest.cpp
M  +5    -5    autotests/highlighter_benchmark.cpp
M  +3    -3    autotests/htmlhighlighter_test.cpp
M  +2    -2    autotests/repository_benchmark.cpp
M  +6    -6    autotests/repository_test.cpp
M  +1    -1    autotests/repository_test_base.cpp
M  +1    -1    autotests/repository_test_base.h
M  +1    -1    autotests/test-config.h.in
M  +6    -6    autotests/testhighlighter.cpp
M  +6    -6    autotests/theme_test.cpp
M  +1    -1    autotests/wildcardmatcher_test.cpp
M  +4    -4    examples/codeeditor/codeeditor.cpp
M  +1    -1    examples/codeeditor/codeeditor.h
M  +3    -3    examples/codepdfprinter/codepdfprinter.cpp
M  +1    -1    examples/codepdfprinter/codepdfprinter.h
M  +5    -5    examples/minimal/main.cpp
M  +4    -4    src/cli/kate-syntax-highlighter.cpp
M  +7    -8    src/lib/CMakeLists.txt
M  +2    -2    src/quick/kquicksyntaxhighlighter.cpp
M  +2    -2    src/quick/kquicksyntaxhighlighter.h
M  +3    -4    src/quick/kquicksyntaxhighlightingplugin.cpp
M  +3    -3    src/quick/repositorywrapper.cpp

https://invent.kde.org/frameworks/syntax-highlighting/commit/c070f3338da63feb0f81170608f9c5de76f98e3c