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

> runtimecontroller.cpp:56
> +    QString findExecutable(const QString& executableName,
> +                           const QStringList& paths = QStringList()) const override {
> +        return QStandardPaths::findExecutable(executableName, paths);

brace on its own line please

> androidruntime.cpp:98
> +    if(paths.empty()) {
> +        auto envPaths = getenv("PATH").split(':');
> +        std::transform(envPaths.begin(), envPaths.end(), rtPaths.begin(),

@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?

> compilerfactories.cpp:51
> +    // no need to check the result, we already do this is CompilerProvider::CompilerProvider()
> +    const QString clang = QStandardPaths::findExecutable(QStringLiteral("clang"));
>  

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

> compilerfactories.cpp:74
>  {
> -    const QString gcc = QStringLiteral("gcc");
> +    const QString gcc = QStandardPaths::findExecutable(QStringLiteral("gcc"));
>  

dito

> compilerfactories.cpp:92
>  {
> -    provider->registerCompiler(createCompiler(name(), QStringLiteral("cl.exe"), false));
> +    provider->registerCompiler(createCompiler(name(), QStandardPaths::findExecutable(QStringLiteral("cl.exe")), false));
>  }

dito

> compilerprovider.cpp:264
>      for ( const CompilerPointer& compiler : m_compilers ) {
> -        const bool absolutePath = QDir::isAbsolutePath(compiler->path());
> -        if ((absolutePath && QFileInfo::exists(rt->pathInHost(Path(compiler->path())).toLocalFile()))
> -            || QStandardPaths::findExecutable( compiler->path(), path).isEmpty() ) {
> +        if (rt->findExecutable(QFileInfo(compiler->path()).fileName()).isEmpty())
>              continue;

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

> dockerruntime.cpp:256
> +
> +    if(paths.empty()) {
> +        auto envPaths = getenv("PATH").split(':');

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"

> flatpakruntime.cpp:235
> +                                       const QStringList& paths) const {
> +    QStringList rtPaths;
> +

dito

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/20190401/6d6924bb/attachment-0001.html>