D5584: Introduction of runtimes

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  overall looks OK to me, mostly nitpicks. I can't comment on the docker/flatpack magic as this goes beyond my knowledge in that area. Also make sure that the docker/flatpak plugins are only installed on linux.

INLINE COMMENTS

> iruntime.h:70
> +     */
> +    virtual Path pathInRuntime(const Path& localPath) = 0;
> +

const?

> iruntime.h:76
> +     */
> +    virtual Path pathInHost(const Path& runtimePath) = 0;
> +

const?

> iruntime.h:84
> +     */
> +    virtual void setEnabled(bool enabled) = 0;
> +

how is this supposed to be used? the API isn't straight-forward, i.e. why is this protected? Why do I call it on this class instead of something like `controller->setEnabled(theRuntime)`?

> iruntimecontroller.h:50
> +    /**
> +     * Makes @p runtimes available to be used.
> +     */

is ownership being transferred here?

> iruntimecontroller.h:52
> +     */
> +    virtual void addRuntimes(const QVector<KDevelop::IRuntime*> &runtimes) = 0;
> +

do we usually add multiple ones in a go? I'd expect when the user adds _a_ runtime, it's only one - so having an add function that does not take a vector would be good to have.

also, are we never removing runtimes?

> backgroundparser.cpp:592
> +    qCDebug(LANGUAGE) << "BackgroundParser::addDocument" << url.toUrl();
> +//     Q_ASSERT(isValidURL(url));
>      QMutexLocker lock(&d->m_mutex);

unrelated change(s) (also below)

> outputexecutejob.cpp:109
>  
> -    Q_ASSERT( d->m_process->state() == QProcess::NotRunning || !killSuccessful );
> +    Q_ASSERT( d->m_process->state() != QProcess::Running || !killSuccessful );
>      delete d;

out of interest: why is this required? what state is the process in?

> outputexecutejob.cpp:307
>      }
> +
>      d->m_status = JobCanceled;

unrelated

> outputexecutejob.h:225
>  
> +    void setExecuteOnHost(bool executeHost);
> +    bool executeOnHost() const;

please add api dox and link to the runtime stuff

> dockerplugin.cpp:59
> +    QProcess* process = new QProcess(this);
> +    connect(process, QOverload<int>::of(&QProcess::finished), this, &DockerPlugin::imagesListFinished);
> +    process->start(QStringLiteral("docker"), {QStringLiteral("images"), QStringLiteral("--filter"), QStringLiteral("dangling=false"), QStringLiteral("--format"), QStringLiteral("{{.Repository}}:{{.Tag}}\t{{.ID}}")}, QIODevice::ReadOnly);

QOverload is 5.8 only, afaik we support older versions - please do the cast manually

> dockerplugin.cpp:75
> +
> +    QProcess* process = qobject_cast<QProcess*>(sender());
> +    Q_ASSERT(process);

if you'd make this whole function a lambda and inline it above into the connect, you could capture the process directly, removing the need to call sender()

> dockerplugin.cpp:88
> +    if (!runtimes.isEmpty())
> +        ICore::self()->runtimeController()->addRuntimes(runtimes);
> +    process->deleteLater();

so the runtime controller takes ownership?

> dockerplugin.cpp:96
> +
> +    for(auto action: actionCollection()->actions())
> +        action->setEnabled(isDocker);

please add braces and a space after `for`

> dockerplugin.cpp:145
> +
> +                    ICore::self()->runtimeController()->addRuntimes({ new DockerRuntime(name) });
> +                });

this is why I was expecting - we want a `addRuntime(new DockerRuntime(name))` here, i.e. no vector

> dockerplugin.h:31
> +{
> +Q_OBJECT
> +public:

indent

> dockerpreferences.h:30
> +    Q_OBJECT
> +    public:
> +        explicit DockerPreferences(KDevelop::IPlugin* plugin, KCoreConfigSkeleton* config, QWidget* parent = nullptr);

from here on until below: deindent one level

> dockerruntime.cpp:53
> +    connect(process, static_cast<void(QProcess::*)(int,QProcess::ExitStatus)>(&QProcess::finished), this, [process, this](int code, QProcess::ExitStatus status){
> +        qDebug() << "inspect" << code << status;
> +        if (code || status)

categorized logging, and output a warning when something goes wrong?

> dockerruntime.cpp:69
> +    if (enable) {
> +        m_userUpperDir = KDevelop::Path(QStandardPaths::writableLocation(QStandardPaths::CacheLocation) + "/docker-" + QString(m_tag).replace('/', '_'));
> +        QDir().mkpath(m_userUpperDir.toLocalFile());

can you add some comments that outline what this is doing?

> dockerruntime.cpp:72
> +
> +        const QStringList cmd = {"pkexec", "bindfs", "--map=root/"+qgetenv("USER"), m_upperDir.toLocalFile(), m_userUpperDir.toLocalFile() };
> +        QProcess p;

qgetenv(USER)? Don't we have KDE api for this? I doubt this would work on windows, but then again - most of the commands here don't work on windows. make sure this whole plugin isn't compiled on windows?

> dockerruntime.cpp:76
> +        p.waitForFinished();
> +        if (p.exitCode())
> +            qCDebug(DOCKER) << "bindfs returned" << m_upperDir << m_userUpperDir << cmd << p.exitCode() << p.readAll();

braces please

> dockerruntime.cpp:95
> +
> +    for(IProject* project: ICore::self()->projectController()->projects()) {
> +        const Path path = project->path();

space

> dockerruntime.cpp:102
> +        const auto ibsm = project->buildSystemManager();
> +        if (ibsm)
> +            ret << "--volume" << QStringLiteral("%1:%2").arg(ibsm->buildDirectory(project->projectItem()).toLocalFile(), buildDir + path.lastPathSegment());

braces

> dockerruntime.cpp:103
> +        if (ibsm)
> +            ret << "--volume" << QStringLiteral("%1:%2").arg(ibsm->buildDirectory(project->projectItem()).toLocalFile(), buildDir + path.lastPathSegment());
> +    }

concat manually instead of using %1:%2?

> dockerruntime.h:29
> +
> +class DockerRuntime : public KDevelop::IRuntime
> +{

I think having a good apidox here is crucial so that others understand what this is doing. i.e. some short explanation of the concepts to get the path mapping etc.

> flatpakruntime.cpp:75
> +{
> +    QDir(m_buildDirectory.toLocalFile()).removeRecursively();
> +}

/me hopes this never deletes $HOME or anything like that accidentally... could this instead use a temporary dir to make sure it's not some pre-existing user-path?

> flatpakruntime.h:29
> +
> +class FlatpakRuntime : public KDevelop::IRuntime
> +{

dito for having apidox outlining the core concepts

> flatpakruntime.h:40
> +
> +    void startProcess(KProcess *process) override;
> +    void startProcess(QProcess *process) override;

why both K and Q process?

> flatpakruntime.h:48
> +    KJob* rebuild();
> +    QList<KJob*> exportBundle(const QString &path);
> +    KJob* executeOnDevice(const QString &host, const QString &path);

const?

> runtimecontroller.cpp:39
> +        connect(process, &QProcess::errorOccurred, this, [process](QProcess::ProcessError error) {
> +            qWarning() << "error!!!" << error << process->program();
> +        });

remove the "error!!!", also below - it's already a warning. but do print the processes error string

REPOSITORY
  R33 KDevPlatform

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

To: apol, #kdevelop, mwolff
Cc: mwolff, anthonyfieroni, kossebau, geetamc, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170514/ea07dafb/attachment-0001.html>