Only add auto-detected compilers to model if they actually exist
ClosedPublic

Authored by arrowd on Apr 14 2017, 9:36 AM.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
arichardson created this revision.Apr 14 2017, 9:36 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptApr 14 2017, 9:36 AM
apol added a subscriber: apol.Apr 14 2017, 12:24 PM

Please, explain the reason for your changes. A commit without explanation is confusion-prone...

I tried to run KDevelop on my system with only clang but not GCC installed it would then select the nonexistant gcc as the compiler. As I made this fix a while ago and just forgot to commit I'm not sure anymore if it then crashed or the parsing was just wrong.

kfunk accepted this revision.Apr 18 2017, 7:28 AM

I think this makes sense.

This revision is now accepted and ready to land.Apr 18 2017, 7:28 AM
skalinichev added inline comments.
languages/plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp
44–46 ↗(On Diff #13429)

It doesn't seem right, we already perform that check in CompilerProvider::CompilerProvider

kfunk requested changes to this revision.Sep 25 2017, 4:14 PM
kfunk added inline comments.
languages/plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp
44–46 ↗(On Diff #13429)

Indeed. That part seems unnecessary.

The change above looks sane, though. The second parameter passed to createCompiler is the *path*, not the *name*, so a fully-qualified path should be passed here.

Maybe we should pass the path as argument to this function instead? Seems more clean to me.

@arichardson Care to double-check?

This revision now requires changes to proceed.Sep 25 2017, 4:14 PM
arrowd commandeered this revision.Jan 3 2019, 2:25 PM
arrowd added a reviewer: arichardson.
arrowd updated this revision to Diff 48598.EditedJan 3 2019, 2:26 PM

Pass absolute path instead of the compiler name to createCompiler(). Remove strange check that was breaking tests.

Original patch by @arichardson.

arrowd added inline comments.Jan 5 2019, 9:11 AM
plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
268

@apol You worked with runtime thingie more than me. What does this check do? Shouldn't it be ! QFileInfo::exists()?

apol added inline comments.Jan 8 2019, 3:50 PM
plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
268

Sounds likely. It's being removed anyway here, right?

Actually this whole patch will break it because it's checking on the host system and not on the runtime.

arrowd updated this revision to Diff 49010.Jan 8 2019, 4:30 PM

Restore a check.

arrowd added inline comments.Jan 8 2019, 4:31 PM
plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
268

I've brought this check back and inverted the condition.

kfunk resigned from this revision.Jan 9 2019, 8:44 AM

@apol Something for you.

apol added inline comments.Jan 9 2019, 7:05 PM
plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
263–264

If you want to keep stored the absolute path, this logic will need to be moved with the new QStandardPaths::findProgram()

arrowd added inline comments.Jan 10 2019, 1:23 PM
plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
263–264

Sorry, I didn't get your comment. That path variable is unused now and can be removed.

apol added inline comments.Jan 10 2019, 5:39 PM
plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
263–264

you didn't port what it did to the new code.

arrowd updated this revision to Diff 49315.Jan 12 2019, 8:26 AM

Restore another check.

arrowd added inline comments.Jan 12 2019, 8:27 AM
plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
263–264

I'd really help me, if you tell exactly what should I fix, since I don't really understand that runtime stuff.

I assumed that I need to check for presence of compiler executable in runtime PATH and fixed that. Please corrent me if I'm wrong.

apol added a comment.Jan 14 2019, 6:35 PM

Let's look at this the other way around, what are you trying to fix?

In D5447#393144, @apol wrote:

Let's look at this the other way around, what are you trying to fix?

createCompiler() function accept full path to a compiler in its 2nd argument. Right now compiler name is passed there. This is what I'm fixing.

mwolff requested changes to this revision.Jan 24 2019, 3:48 PM

the path of the compiler is now always the path of the compiler in the local host, which breaks when you have a runtime with a different setup. I think the changes that use QStandardPaths in registerDefaultCompilers should be undone and we should instead check the validity better elsewhere.

So, the question really is: Why was the code inloc 213 in compilerprovider.cpp not working for you? If gcc wasn't installed, we should get:

  • compiler->path() == "gcc", i.e. not absolute
  • QStandardpaths::findExecutable("gcc", path) should always return false for all the entries in your PATH
  • we should thus never set the gcc compiler

please debug why the above didn't work for you

This revision now requires changes to proceed.Jan 24 2019, 3:48 PM

the path of the compiler is now always the path of the compiler in the local host, which breaks when you have a runtime with a different setup.

Hum. Before being passed to rt->pathInHost(), the compiler path gets wrapped into KDevelop::Path. Doesn't this make it absolute/invalid? Judging from Path::Path(QString) constructor, it should't even accept things like "gcc".

And what are we checking there? I still don't make any sense from this check.

So, the question really is: Why was the code inloc 213 in compilerprovider.cpp not working for you? If gcc wasn't installed, we should get:

  • compiler->path() == "gcc", i.e. not absolute
  • QStandardpaths::findExecutable("gcc", path) should always return false for all the entries in your PATH
  • we should thus never set the gcc compiler

    please debug why the above didn't work for you

Are you talking about old code? Then it is not QStandardpaths::findExecutable("gcc", path) that doesn't work for me, it is QFileInfo::exists(rt->pathInHost()) part. Why do we skip adding a compiler if it does exist?

the path of the compiler is now always the path of the compiler in the local host, which breaks when you have a runtime with a different setup.

Hum. Before being passed to rt->pathInHost(), the compiler path gets wrapped into KDevelop::Path. Doesn't this make it absolute/invalid? Judging from Path::Path(QString) constructor, it should't even accept things like "gcc".

And what are we checking there? I still don't make any sense from this check.

you are right, this code should essentially do if (rt->findExecutable(compiler->path().isEmpty()) continue;, could you introduce that please?

So, the question really is: Why was the code inloc 213 in compilerprovider.cpp not working for you? If gcc wasn't installed, we should get:

  • compiler->path() == "gcc", i.e. not absolute
  • QStandardpaths::findExecutable("gcc", path) should always return false for all the entries in your PATH
  • we should thus never set the gcc compiler

    please debug why the above didn't work for you

Are you talking about old code? Then it is not QStandardpaths::findExecutable("gcc", path) that doesn't work for me, it is QFileInfo::exists(rt->pathInHost()) part. Why do we skip adding a compiler if it does exist?

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

you are right - the issue is that the old code skips compilers when they exist - there's a ! missing in front of the parens here probably

arrowd added a comment.Feb 3 2019, 4:56 PM

you are right, this code should essentially do if (rt->findExecutable(compiler->path().isEmpty()) continue;, could you introduce that please?

I got lost in this conversation. Let's break down all these checks.

Old code:

  • absolutePath. Always true now, so can be dropped.
  • QFileInfo::exists(rt->pathInHost()). Should have the condition flipped. Should check if a given absolute path to the compiler exists inside a runtime.
  • QStandardPaths::findExecutable(). If I get it right, it should search for given executable name in runtime's PATH paths, but actually was searching in host filesystem.

New code:

  • No absolutePath.
  • Fixed QFileInfo::exists(rt->pathInHost()). It now checks if given absolute path in the host also exists in the runtime.
  • QStandardPaths::findExecutable(). Still does something strange.

So, what's required to get this in? Fix QStandardPaths::findExecutable() to search inside runtime?

you are right, this code should essentially do if (rt->findExecutable(compiler->path().isEmpty()) continue;, could you introduce that please?

I got lost in this conversation. Let's break down all these checks.

Old code:

  • absolutePath. Always true now, so can be dropped.

It must not be absolute, the compiler must be looked up within the runtime! If we pass in an absolute path resolved on the local host, it will break the runtime integration.

  • QFileInfo::exists(rt->pathInHost()). Should have the condition flipped. Should check if a given absolute path to the compiler exists inside a runtime.

No, it should check if an executable of the given name exists in the runtime.

  • QStandardPaths::findExecutable(). If I get it right, it should search for given executable name in runtime's PATH paths, but actually was searching in host filesystem.

Yes, QStandardPaths obviously doesn't know anything about the kdevelop runtimes.

New code:

  • No absolutePath.

Yes.

  • Fixed QFileInfo::exists(rt->pathInHost()). It now checks if given absolute path in the host also exists in the runtime.

rt->findExecutable(name) which returns empty when no such executable is found

  • QStandardPaths::findExecutable(). Still does something strange.

Should be used by the default/local runtime internally

So, what's required to get this in? Fix QStandardPaths::findExecutable() to search inside runtime?

No, expand the runtime API to add an findExecutable similar to the pathInHost API there, then use it here

I started working on it.

I added method declaration to kdevplatform/interfaces/iruntime.h and wanted to add a default implementation in iruntime.cpp, but it turned out that to use KDevelop::Path, I have to include <util/path.h>, which belongs to KDevPlatformUtil library. I take it, KDevPlatformInterfaces isn't allowed to link to KDevPlatformUtil? How should I proceed?

I started working on it.

I added method declaration to kdevplatform/interfaces/iruntime.h and wanted to add a default implementation in iruntime.cpp, but it turned out that to use KDevelop::Path, I have to include <util/path.h>, which belongs to KDevPlatformUtil library. I take it, KDevPlatformInterfaces isn't allowed to link to KDevPlatformUtil? How should I proceed?

Don't supply a default implementation, interfaces should be pure (we have some default implementations there, but really that shouldn't be the case)

arrowd updated this revision to Diff 55196.Apr 1 2019, 1:46 PM

Add IRuntime::findExecutable() API and make compilerprovider use it.

Tests seems to not regress.

mwolff requested changes to this revision.Apr 1 2019, 4:41 PM
mwolff added inline comments.
kdevplatform/shell/runtimecontroller.cpp
56

brace on its own line please

plugins/android/androidruntime.cpp
98

@arrowd the PATH list here should still be going through pathInHost, otherwise it will point to the wrong directories. so it would be something like:

QStringList rtPaths = paths;
if (rtPaths.isEmpty()) {
    // transform "PATH" env var values into rtPaths
}
// lookup using rtPaths

@apol how would you implement this?

plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp
51 ↗(On Diff #55196)

shouldn't be required, we find the executable internally by name after all

74 ↗(On Diff #55196)

dito

92 ↗(On Diff #55196)

dito

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

remove the QFileInfo and .fileName() here, just use the path like previously. note how the path could be an absolute path not inside the PATH, your patch would break this

plugins/docker/dockerruntime.cpp
256

all I said in the android runtime applies here too I believe

also, the code is identical, so share it trough some "runtime helper header"

plugins/flatpak/flatpakruntime.cpp
235

dito

This revision now requires changes to proceed.Apr 1 2019, 4:41 PM
arrowd updated this revision to Diff 55211.Apr 1 2019, 5:10 PM
arrowd marked an inline comment as done.
  • Move the bracket.
arrowd added inline comments.Apr 1 2019, 5:10 PM
plugins/android/androidruntime.cpp
98

getenv there is IRuntime::getenv(). If I understand it right, it returns paths inside the RT, just what we need.

plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp
51 ↗(On Diff #55196)

Hmm. But the second arg of createCompiler is called path, and whole this patch is about passing a full path there.

Probably, in presence of such thing as runtimes, we need to revise Compiler's API and do something about path() method?

plugins/docker/dockerruntime.cpp
256

also, the code is identical, so share it trough some "runtime helper header"

Where to put it?

mwolff requested changes to this revision.Apr 14 2019, 8:41 AM
mwolff added inline comments.
kdevplatform/interfaces/iruntime.h
85

you can remove the paths arg - it's currently never used as far as I can see. we can add it if need be in the future

plugins/android/androidruntime.cpp
98

yes, a path in the runtime is by definition not a pathInHost. So getenv("PATH") will give use e.g. /usr/bin but then we need to map that into, say, /opt/android/something/usr/bin, no?

also, I just notice: please use QByteArrayLiteral around "PATH"

plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp
51 ↗(On Diff #55196)

afaik it's called path since it can be a user-provided absolute path. But the builtin compiler defaults all want to do dynamic lookups. so no, I think it's fine to keep the API but again - don't hardcode an absolute path in the host here - it should be a name that's checked in either the runtime or the host

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

see this comment again

plugins/docker/dockerruntime.cpp
253

open brace on its own line

256

hmm good question. but see my comment above - since paths is never filled, we can simplify this code a lot and then the code duplication is potentially not that bad anymore.

but I would be OK with a trivial runtimehelpers.h with an inline implementation of this function for now. bonus points if you make this a proper static lib

258

all duplicated code paths that use transform will crash: you have to resize rtPaths to the correct size, or use a back_inserter

This revision now requires changes to proceed.Apr 14 2019, 8:41 AM
arrowd updated this revision to Diff 57676.May 6 2019, 6:24 PM
arrowd marked 7 inline comments as done.
  • Address some comments.
arrowd added inline comments.May 6 2019, 6:26 PM
plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp
51 ↗(On Diff #55196)

So, this hunk and other dittos should be completely reversed?

mwolff added inline comments.May 7 2019, 12:12 PM
plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp
51 ↗(On Diff #55196)

yes exactly

arrowd updated this revision to Diff 57726.May 7 2019, 3:12 PM
arrowd marked 11 inline comments as done.
  • Use compiler executable name instead of absolute path.
arrowd added inline comments.May 7 2019, 3:12 PM
plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp
266

In other comment you said

afaik it's called path since it can be a user-provided absolute path.

So, if compiler->path() can return absolute path, is it OK to pass it as is into rt->findExecutable()?

mwolff accepted this revision.Sep 20 2019, 4:13 AM

sorry for the super long delay. I like what I'm seeing - if this works, please commit

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

Yes, an absolute path will then just be resolved directly within the RT without looking at the PATH env var of the RT

This revision is now accepted and ready to land.Sep 20 2019, 4:13 AM