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

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> iruntime.h:85
> +    virtual QString findExecutable(const QString& executableName,
> +                                   const QStringList& paths = QStringList()) const = 0;
> +

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

> arrowd wrote in androidruntime.cpp:98
> `getenv` there is `IRuntime::getenv()`. If I understand it right, it returns paths inside the RT, just what we need.

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"`

> arrowd wrote in compilerfactories.cpp:51
> 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?

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

> mwolff wrote in compilerprovider.cpp:264
> 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

see this comment again

> dockerruntime.cpp:253
> +QString DockerRuntime::findExecutable(const QString& executableName,
> +                                       const QStringList& paths) const {
> +    QStringList rtPaths;

open brace on its own line

> arrowd wrote in dockerruntime.cpp:256
> > also, the code is identical, so share it trough some "runtime helper header"
> 
> Where to put it?

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

> dockerruntime.cpp:258
> +        auto envPaths = getenv("PATH").split(':');
> +        std::transform(envPaths.begin(), envPaths.end(), rtPaths.begin(),
> +                       [](QByteArray p) {

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

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D5447

To: arrowd, mwolff, arichardson
Cc: skalinichev, apol, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20190414/4e2ff310/attachment-0001.html>