Make sure we use the same compiler settings as the project is by default
ClosedPublic

Authored by apol on Mar 7 2018, 4:57 PM.

Details

Summary

Instead of trusting the user to configure it beforehand, make it
possible to check which compiler is being used in the build manager and
uses it to figure out the initial compilation settings.
This includes a QStringList IBSM::findCompiler(item) method to get the
path to the used compiler. I wonder if we should be providing the
sysroot as well.

Test Plan

Been playing around with the androidqt branch that uses docker
to implement the SDK instead of just expecting the user to install it himself.

Diff Detail

Repository
R32 KDevelop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
apol created this revision.Mar 7 2018, 4:57 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMar 7 2018, 4:57 PM
apol requested review of this revision.Mar 7 2018, 4:57 PM
mwolff requested changes to this revision.Mar 8 2018, 9:27 AM
mwolff added a subscriber: mwolff.

cool! but needs some work :)

kdevplatform/project/interfaces/ibuildsystemmanager.h
137

make it pure virtual like the rest of this interface

rename to compiler

plugins/cmake/cmakemanager.cpp
1004

category-based logging, also below

1011

remove ...

1014

use QLatin1String since you are concatenating

plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
41

move up, add interfaces/

157

this isAbsolutePath check - shouldn't it be an assertion? the BSM should sanitize and produce absolute paths, I'd say (like, based on the build/source dir)

162

for (const auto& compiler : m_compilers)

or better yet:

if (std::any_of(m_compilers.begin(), m_compilers.end(),
    [pathString](const auto& compiler) { compiler->path() == pathString; })
{
    continue;
}
171

this looks weird to me - this shouldn't be hardcoded like this, no? it should go through all factories and find a match. I.e. on windows we may need to use the msvc factory?

This revision now requires changes to proceed.Mar 8 2018, 9:27 AM
apol marked 6 inline comments as done.Mar 8 2018, 2:32 PM
apol updated this revision to Diff 29007.Mar 8 2018, 2:55 PM

Address comments by Milian

mwolff requested changes to this revision.Mar 8 2018, 3:12 PM
mwolff added inline comments.
plugins/custom-buildsystem/custombuildsystemplugin.cpp
201

this is wrong, you'll have to introduce another key for this, building could e.g. be "make" and not the compiler itself

feel free to leave this not-implemented and return nothing for now, and then get some sane default in the IADM

plugins/custom-definesandincludes/compilerprovider/compilerfactories.h
35

here and below: style: space before &

plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
155

I find it odd that this is the only place where this new API is being used, no? shouldn't it also be used elswhere? like, when we don't really use runtimes at all?

157

the ! is wrong, no? otherwise you only run the code below when the path is empty, which sounds odd

168

no need for the else + indentation, you continue above

plugins/custommake/custommakemanager.cpp
329

this should not return anything and let the IADM figure out a sane default

and we should probably add a TODO to expand the custom make manager to allow setting the compiler for a given target

plugins/qmakemanager/qmakemanager.cpp
519

this is wrong, it should query the item's cache for the QMAKE_CXX variable

plugins/qmakemanager/qmakemanager.h
86

style: space before *

This revision now requires changes to proceed.Mar 8 2018, 3:12 PM
apol updated this revision to Diff 29021.Mar 8 2018, 4:19 PM

Don't try to guess what's best for ibsm implementations other than cmake for now
Use this information whenever a project is opened or reconfigured.
Make sure we report a configuration change after cmake has reconfigured after a runtime change.

mwolff added a comment.Mar 8 2018, 4:35 PM

one minor nit (the qGetEnv), otherwise lgtm now

plugins/cmake/cmakemanager.cpp
320

thinking ahead: we should think about moving this reparse to a central place, connected to the projectConfigurationChanged signal above

plugins/custommake/custommakemanager.cpp
329

return empty

apol marked an inline comment as done.Mar 8 2018, 4:57 PM
apol added inline comments.
plugins/cmake/cmakemanager.cpp
320

Thought about it too, still I feel more comfortable having it here.

apol updated this revision to Diff 29023.Mar 8 2018, 4:59 PM

Forgot to remove a dumb implementation and other last touches.

mwolff accepted this revision.Mar 8 2018, 10:12 PM

three minor nits, otherwise lgtm

plugins/custom-buildsystem/custombuildsystemplugin.cpp
202

remove the second empty line

plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
169

join next line

plugins/qmakemanager/qmakemanager.cpp
521

remove second empty line

This revision is now accepted and ready to land.Mar 8 2018, 10:12 PM
mwolff requested changes to this revision.Mar 8 2018, 10:13 PM

actually - wait a minute - this will make use use GCC in even more places... leading to lots of potential issues!

This revision now requires changes to proceed.Mar 8 2018, 10:13 PM

ugh sorry, I apparently just thought about this for real the first time... people (like me sometimes) often use GCC for compiling. But our language support is notoriously _bad_ at trying to cope with the gcc compiler environment. So we may actually now see *more* issues than before, no? Do we really want this? Hmm I was actually thinking about _removing_ most of the non-clang compiler stuff (builtin macros, includes etc) - but I'd like to discuss this first

Hmm I was actually thinking about _removing_ most of the non-clang compiler stuff (builtin macros, includes etc) - but I'd like to discuss this first

+1 on that. I never understood why we're trying to emulate another compiler, when we clearly parse with Clang and not GCC or MSVC. It also doesn't seem to work 100%, since we can't make Clang forget that it is Clang, so it reads stuff with #ifdef __clang__ anyway. For me this always looked like something that made sense in the pre-Clang (KDevelop 4) days, but not anymore.

@aaronpuchert The idea was originally to give the user the choice between what "view" of the source he gets. In principle what Aleix implemented here would be ideal - the editor shows what will actually get compiled. But, sadly, reality differs and hits us hard when we run into all these issues :(

The Clang developers work really hard on compatibility with both GCC and MSVC, but I don't think they want to use the same defines and includes for that. For example, both Clang and GCC have their own private header files (on my machine, /usr/lib64/gcc/x86_64-suse-linux/7/include and /usr/lib64/clang/5.0.1/include/). These are quite similar, but not interchangeable. I don't think one can use one set of headers with the other compiler.

For the most part, I also use GCC as compiler, but I configure KDevelop to behave like Clang. Yes, my compiler will see something slightly different. So what? What matters is that both GCC and KDevelop's libclang parser implement the same C++ standard.

Since there is clang-cl, I assume that Clang can also imitate MSVC to some extent. But I don't think spying on cls defines and includes and feeding them into libclang is necessary or sufficient for that. (Consider MS inline assembly, which Clang understands - but not by setting some defines.)

apol marked 3 inline comments as done.Mar 9 2018, 3:53 PM
apol added a comment.Mar 9 2018, 3:55 PM

ugh sorry, I apparently just thought about this for real the first time... people (like me sometimes) often use GCC for compiling. But our language support is notoriously _bad_ at trying to cope with the gcc compiler environment. So we may actually now see *more* issues than before, no? Do we really want this? Hmm I was actually thinking about _removing_ most of the non-clang compiler stuff (builtin macros, includes etc) - but I'd like to discuss this first

Well the whole point here is to make sure that we are running the same as the project. So if the alternative is using /opt/android-ndk/...../something-gcc or /usr/bin/clang. Yes, we want gcc.

apol added a comment.Mar 9 2018, 3:55 PM

The Clang developers work really hard on compatibility with both GCC and MSVC, but I don't think they want to use the same defines and includes for that. For example, both Clang and GCC have their own private header files (on my machine, /usr/lib64/gcc/x86_64-suse-linux/7/include and /usr/lib64/clang/5.0.1/include/). These are quite similar, but not interchangeable. I don't think one can use one set of headers with the other compiler.

For the most part, I also use GCC as compiler, but I configure KDevelop to behave like Clang. Yes, my compiler will see something slightly different. So what? What matters is that both GCC and KDevelop's libclang parser implement the same C++ standard.

Since there is clang-cl, I assume that Clang can also imitate MSVC to some extent. But I don't think spying on cls defines and includes and feeding them into libclang is necessary or sufficient for that. (Consider MS inline assembly, which Clang understands - but not by setting some defines.)

I'll make sure it's not clang-cl.

apol updated this revision to Diff 29096.Mar 9 2018, 3:58 PM

Address style issues.
Take clang-cl into account

apol added a comment.Mar 14 2018, 3:28 PM

bump?
I'd say it's either this or remove the concept of choosing where this information come from and allow the project manger to provide this information (which IMHO is ideal, but then this was created for some reason).

mwolff accepted this revision.Mar 14 2018, 5:47 PM

stylistic wise a clear +1, conceptually I still fear this is going to break a lot of things - but not due to your code, but rather due to the broken GCC compatibility in clang. I think we can get this in and then we need to figure out how to handle the clang/gcc compatibility story somehow else... I don't know how, nor do I have time to investigate right now :(

Can you just do me one favor please, before you merge: Can you try KDevelop with this patch on a non-cross-compiled CMake project where you use GCC instead of clang for compiling? Does KDevelop work fine in its current form with a recent clang/gcc - or does it break in some horrible ways? If it's OK then I'd say let's merge this. If everything breaks... I fear we need to figure out a workaround first before introducing the breakage to everyone :(

This revision is now accepted and ready to land.Mar 14 2018, 5:47 PM
apol added a comment.Mar 15 2018, 1:31 AM

stylistic wise a clear +1, conceptually I still fear this is going to break a lot of things - but not due to your code, but rather due to the broken GCC compatibility in clang. I think we can get this in and then we need to figure out how to handle the clang/gcc compatibility story somehow else... I don't know how, nor do I have time to investigate right now :(

Can you just do me one favor please, before you merge: Can you try KDevelop with this patch on a non-cross-compiled CMake project where you use GCC instead of clang for compiling? Does KDevelop work fine in its current form with a recent clang/gcc - or does it break in some horrible ways? If it's OK then I'd say let's merge this. If everything breaks... I fear we need to figure out a workaround first before introducing the breakage to everyone :(

I don't see how it could break horribly, in the end it's just passing some includes and defines that IIRC we lived without for longtime in kdev4 with reasonable success.
I commented out the clang option on my system for a bit and will run it like that for the day tomorrow, let's see if I find any odd issues.

see also: https://bugs.kde.org/show_bug.cgi?id=387005

In that bug report we (presumably) get bitten by clang-5.0 (which is used for parsing) failing to understand clang-3.8 system headers (autodetected compiler).
I second @aaronpuchert - it would be better to remove support for other compilers than the one that is actually used for parsing.

apol added a comment.Apr 18 2018, 12:35 PM

see also: https://bugs.kde.org/show_bug.cgi?id=387005

In that bug report we (presumably) get bitten by clang-5.0 (which is used for parsing) failing to understand clang-3.8 system headers (autodetected compiler).
I second @aaronpuchert - it would be better to remove support for other compilers than the one that is actually used for parsing.

If that's the case, I suggest someone to suggest a plan towards deprecating custom-definesandincludes.

Everyone, please test https://phabricator.kde.org/D12331

I hope that this improves the situation and then we can merge this patch here on top of it.

In D11136#248743, @apol wrote:

I second @aaronpuchert - it would be better to remove support for other compilers than the one that is actually used for parsing.

If that's the case, I suggest someone to suggest a plan towards deprecating custom-definesandincludes.

The custom- part is obviously still Ok. But I think simulating another compiler is beyond the scope of libclang and will never completely work.

ping? I think it would still make sense to merge this - Aleix, are you still running this? Should we revive the patch, rebase it and get it in?

apol added a comment.Feb 11 2019, 4:19 PM

I'll rebase over the week, I wonder why it fell through.

apol closed this revision.Feb 11 2019, 5:03 PM