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.
Details
- Reviewers
mwolff - Group Reviewers
KDevelop - Commits
- R32:62ae1929c5a5: Make sure we use the same compiler settings as the project is by default
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
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? |
plugins/custom-buildsystem/custombuildsystemplugin.cpp | ||
---|---|---|
201 ↗ | (On Diff #29007) | 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 ↗ | (On Diff #29007) | here and below: style: space before & |
plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp | ||
155 ↗ | (On Diff #29007) | 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 ↗ | (On Diff #29007) | the ! is wrong, no? otherwise you only run the code below when the path is empty, which sounds odd |
168 ↗ | (On Diff #29007) | no need for the else + indentation, you continue above |
plugins/custommake/custommakemanager.cpp | ||
329 ↗ | (On Diff #29007) | 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 ↗ | (On Diff #29007) | this is wrong, it should query the item's cache for the QMAKE_CXX variable |
plugins/qmakemanager/qmakemanager.h | ||
86 ↗ | (On Diff #29007) | style: space before * |
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.
plugins/cmake/cmakemanager.cpp | ||
---|---|---|
320 | Thought about it too, still I feel more comfortable having it here. |
three minor nits, otherwise lgtm
plugins/custom-buildsystem/custombuildsystemplugin.cpp | ||
---|---|---|
202 ↗ | (On Diff #29007) | remove the second empty line |
plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp | ||
202 ↗ | (On Diff #29007) | join next line |
plugins/qmakemanager/qmakemanager.cpp | ||
521 ↗ | (On Diff #29007) | remove second empty line |
actually - wait a minute - this will make use use GCC in even more places... leading to lots of potential issues!
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
+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.)
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.
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).
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.
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.
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?