Fixed dependency order in build system
ClosedPublic
Actions

Authored by obogdan on Nov 30 2016, 6:53 PM.

Details

Reviewers

patrickelectric
kfunk

Group Reviewers

KDevelop

Commits

R185:0c6eb4e01426: SVN_SILENT made messages (.desktop file) - always resolve ours

Summary

The dependency wasn't set properly and the build of embeddedlauncher.cpp failed because the ui_firsttimewizard.h wasn't generated yet.

Test Plan

I've compiled and installed this to ~/KDE/install and done:

export QT_PLUGIN_PATH=$QT_PLUGIN_PATH:$HOME/KDE/install/lib/x86_64-linux-gnu/plugins
export XDG_DATA_DIRS=$XDG_DATA_DIRS:$HOME/KDE/install/lib/x86_64-linux-gnu/plugins  
kdevelop

but KDevelop did not see the plugin. I don't know how better to test this.

Diff Detail

Repository

R185 KDevelop Plugin: Embedded Platforms

Branch

fix-buildsystem (branched from master)

Lint

No Linters Available

Unit

No Unit Test Coverage

obogdan updated this revision to Diff 8641.Nov 30 2016, 6:53 PM

obogdan retitled this revision from to Fix build system issues.

obogdan updated this object.

obogdan edited the test plan for this revision. (Show Details)

obogdan added a reviewer: KDevelop.

obogdan set the repository for this revision to R185 KDevelop Plugin: Embedded Platforms.

obogdan added a project: KDevelop.

obogdan added a subscriber: KDevelop.

Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptNov 30 2016, 6:53 PM

Rest LGTM

CMakeLists.txt
14	Add minimum version while at it: 5.0
arduinoversion.h.in
2	Add header include guards

kfunk requested changes to this revision.Nov 30 2016, 7:09 PM

kfunk added a reviewer: kfunk.

This revision now requires changes to proceed.Nov 30 2016, 7:09 PM

KDevPlatform required version set to 5.0
Protected include for arduinoversion.h

obogdan marked 2 inline comments as done.Nov 30 2016, 8:57 PM

obogdan added inline comments.

CMakeLists.txt
14	Done
arduinoversion.h.in
2	Done

obogdan marked 2 inline comments as done.Nov 30 2016, 8:58 PM

Feel free to push after fixing those style issues

CMakeLists.txt
67	Move closing paren to front
95	Style: Indent off
arduinoversion.h.in
2	Style: Naming scheme is usually like this: `ARDUINOVERSION_H`

This revision is now accepted and ready to land.Nov 30 2016, 9:02 PM

obogdan added a reviewer: patrickelectric.Nov 30 2016, 9:07 PM

Style fix

I'd like to test this before commiting it. Please advise on how can I do that, since what I've tried so far hasn't worked.

patrickelectric requested changes to this revision.Nov 30 2016, 9:42 PM

patrickelectric edited edge metadata.

patrickelectric added inline comments.

arduinoversion.h.in
2	Use pragma once.

This revision now requires changes to proceed.Nov 30 2016, 9:42 PM

@obogdan Have you installed the plugin ?

cmake .. -DCMAKE_INSTALL_PREFIX=PathToKDE5
make install

kfunk accepted this revision.Nov 30 2016, 9:47 PM

kfunk edited edge metadata.

kfunk added inline comments.

arduinoversion.h.in
2	We don't use `#pragma once` in KDevelop usually (it's also a non-standard extension). Please use standard include guards.

This is my test compile command:

rm -rf * && cmake -DCMAKE_INSTALL_PREFIX=~/KDE/install ../../source/kdev-embedded && make && make install

The plugin is installed. However KDevelop does not see it. I have system installed KDevelop 5.0.1.

You shouldn't use in-source builds, they are terrible. Apart from that, did you add the install directory to $QT_PLUGIN_PATH?

brauch added a comment.Nov 30 2016, 10:21 PM

This comment was removed by brauch.

... sorry, disregard the in-source part, it's late ;)

In D3551#66298, @brauch wrote:

You shouldn't use in-source builds, they are terrible. Apart from that, did you add the install directory to $QT_PLUGIN_PATH?

I did add it. Please read the Test plan from the Review Request description.

obogdan marked an inline comment as done.Nov 30 2016, 10:33 PM

obogdan added inline comments.

arduinoversion.h.in
2	I agree that we should use standard ways of doing things, and keeping the style across the project. Considering this, should I replace `#pragma once` across the project with `#ifndef`s ?

obogdan marked 2 inline comments as done.Nov 30 2016, 10:34 PM

Updating from arc (no changes)

kfunk added inline comments.Nov 30 2016, 11:12 PM

arduinoversion.h.in
2	Please do, but in a separate commit

kfunk accepted this revision.Nov 30 2016, 11:13 PM

kfunk edited edge metadata.

patrickelectric accepted this revision.Nov 30 2016, 11:55 PM

patrickelectric edited edge metadata.

patrickelectric added inline comments.

arduinoversion.h.in
2	@obogdan, disregard my last comment, use #ifdef instead (link).

This revision is now accepted and ready to land.Nov 30 2016, 11:55 PM

obogdan marked 5 inline comments as done.Dec 1 2016, 8:01 AM

obogdan retitled this revision from Fix build system issues to Fixed dependency order in build system.Dec 1 2016, 8:11 AM

obogdan edited edge metadata.

Pushed with commit: https://phabricator.kde.org/R185:be89e904a8ced5ca9b6ca83639f335bf02747568

Revision Contents
Changeset List

		Path
M		.gitignore (1 line)
M		CMakeLists.txt (22 lines)
A	M	arduinoversion.h.in (8 lines)
V		{firsttimewizard.h ← firsttimewizard.h.in} (4 lines)
V		{firsttimewizard.h.in → firsttimewizard.h}
M		launcher/embeddedlauncher.cpp (2 lines)
V		{toolkit.h ← toolkit.h.in} (2 lines)
V		{toolkit.h.in → toolkit.h}

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	8641			Nov 30 2016, 6:43 PM	★	★
Diff 2	8645			Nov 30 2016, 8:55 PM	★	★
Diff 3	8648			Nov 30 2016, 9:33 PM	★	★
Diff 4	8652	373d9c8	Updating from arc (no changes)	Nov 30 2016, 10:40 PM	★	★