Details
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.
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.
| 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 |
| 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? |
Pass absolute path instead of the compiler name to createCompiler(). Remove strange check that was breaking tests.
Original patch by @arichardson.
| plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp | ||
|---|---|---|
| 214 ↗ | (On Diff #66884) | 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. |
| plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp | ||
|---|---|---|
| 214 ↗ | (On Diff #66884) | I've brought this check back and inverted the condition. |
| 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() | |
| 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. | |
| plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp | ||
|---|---|---|
| 263–264 | you didn't port what it did to the new code. | |
| 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. | |
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.
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
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?
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 youAre 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 | ||
|---|---|---|
| 214 ↗ | (On Diff #66884) | 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 |
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?
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?
Don't supply a default implementation, interfaces should be pure (we have some default implementations there, but really that shouldn't be the case)
Add IRuntime::findExecutable() API and make compilerprovider use it.
Tests seems to not regress.
| 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 | ||
| 52 ↗ | (On Diff #57726) | shouldn't be required, we find the executable internally by name after all |
| 74 ↗ | (On Diff #57726) | dito |
| 92 ↗ | (On Diff #57726) | 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 | |
| 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 | ||
| 52 ↗ | (On Diff #57726) | 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 |
Where to put it? | |
| 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 | ||
| 52 ↗ | (On Diff #57726) | 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 | |
| plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp | ||
|---|---|---|
| 52 ↗ | (On Diff #57726) | So, this hunk and other dittos should be completely reversed? |
| plugins/custom-definesandincludes/compilerprovider/compilerfactories.cpp | ||
|---|---|---|
| 52 ↗ | (On Diff #57726) | yes exactly |
| plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp | ||
|---|---|---|
| 266 | In other comment you said
So, if compiler->path() can return absolute path, is it OK to pass it as is into rt->findExecutable()? | |
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 | |