Differential D5188

[WIP] Basic support for building Go projects.
ClosedPublic
Actions

Authored by ematirov on Mar 26 2017, 6:53 PM.

Details

Reviewers

mwolff

Group Reviewers

KDevelop

Commits

R59:b4de161565e9: Basic support for building Go projects.

Summary

Initial work on build support (code is mostly based on CustomMakeBuild plugin).
For testing, open a folder with golang project and choose "Go project manager".

Known bugs:

~~No "build" \ "install" \ "clean" buttons on right-click menu in projects view. However, "Build selection" works.~~ Fixed, now project folders are "build folders" so they are buildable.
~~No error messages from Golang in case something is wrong - only exit code is shown. (Could be easily tested with running "install" in case project is not in GOPATH)~~ Fixed.
Probably something more.

Another ToDos:

Add support for configuring GOPATH and go executable path

This revision is work in progress and was created to share status and receive suggestions \ nitpicks.
So, any opinions are welcome!

Test Plan

Manual testing by opening project with manager "KDevGoBuildSystem" and trying to build\install\clean.

Diff Detail

Repository

R59 KDevelop Go

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

ematirov created this revision.Mar 26 2017, 6:53 PM

Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMar 26 2017, 6:53 PM

apol added a subscriber: apol.Mar 27 2017, 11:40 AM

apol added inline comments.

CMakeLists.txt
32	I'd call it "projectmanager" or "buildsystem". It's th go support, it's a given.
go-buildsystem/gobuilder.cpp
39 ↗	(On Diff #12841)	Just use "go" so it's looked up in PATH. Will fix it in special set ups.

mwolff requested changes to this revision.Mar 27 2017, 8:04 PM

mwolff added a subscriber: mwolff.

mwolff added inline comments.

go-buildsystem/gobuilder.cpp
20 ↗	(On Diff #12841)	here and below: wrap in QStringLiteral
44 ↗	(On Diff #12841)	shouldn't be required
go-buildsystem/gobuilder.h
24 ↗	(On Diff #12841)	why do you call these arguments `dom`?
go-buildsystem/gobuildsystem.cpp
50 ↗	(On Diff #12841)	`return {}`
55 ↗	(On Diff #12841)	`return {}`
60 ↗	(On Diff #12841)	`return {}`
97 ↗	(On Diff #12841)	this looks like it should be a do-while auto project = item->project(); // remember for below ProjectFolderItem *folder = nullptr; do { folder = dynamic_cast<...>(item); item = item->parent(); } while (!folder && item); if (folder) { return folder->path(); } else { return project->path(); }
104 ↗	(On Diff #12841)	this can crash. note how your loop above theoretically can run until both item and fi are null, then you'd go into this branch and crash
111 ↗	(On Diff #12841)	`return {}`

This revision now requires changes to proceed.Mar 27 2017, 8:04 PM

Fix review suggestions. Make folders of project buildable by right-clicking.
Suggestions about naming are welcome - I renamed folder to buildsystem but classes should have Go prefix I think, for easier debug \ etc. (It looks strange to make break BuildSystem::methodName without prefix IMHO). However, I'm not sure if files should contains that "Go" prefix.

can go project be detected based on some file? If so, could you add something like this to the json file please:

"X-KDevelop-ProjectFilesFilter": [
    "CMakeLists.txt"
],

This is from the cmake manager, you could try to use "*.go", if there's no real "project" file in the go world.

Ping? I think other than the mimetype issue, this is OK to get merged. Do you have commit rights?

This revision is now accepted and ready to land.May 14 2017, 10:16 AM

In D5188#109400, @mwolff wrote:

Ping? I think other than the mimetype issue, this is OK to get merged. Do you have commit rights?

Sorry, I was busy studying.
I am planning to work on this proposal more during GSoC coding period and I am not sure that this one should be pushed before further polishing.
There is no "project file" in Go, but specifying "*.go" as mimetype works perfect.
I'll try to add output from "go build" to "Build" output window and will update RR with that.

In D5188#110152, @ematirov wrote:

There is no "project file" in Go, but specifying "*.go" as mimetype works perfect.

Doesn't Go has its own build system? This should serve as "project file", like Makefiles or CMakeLists.txt for C/C++.

In D5188#110166, @arrowdodger wrote:

In D5188#110152, @ematirov wrote:

There is no "project file" in Go, but specifying "*.go" as mimetype works perfect.

Doesn't Go has its own build system? This should serve as "project file", like Makefiles or CMakeLists.txt for C/C++.

Go's build system uses files layout (see https://golang.org/doc/code.html and https://github.com/golang/go/wiki/GithubCodeLayout). But even in this case user isn't forced to place all project files in GOPATH, afaik, it's possible to use "go build", "go run", "go install" in any directory containing go project even if it's not in GOPATH. So, "Go project" is just a directory with bunch of files and sub-directories.

Set tool view for build job.
Add project files filter

What needs to happen to have this merged?

In D5188#117158, @apol wrote:

What needs to happen to have this merged?

Probably, nothing. I'll merge it then.

Closed by commit R59:b4de161565e9: Basic support for building Go projects. (authored by ematirov). · Explain WhyJun 19 2017, 12:28 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

		Path
M		CMakeLists.txt (1 line)
A	M	buildsystem/CMakeLists.txt (14 lines)
A	M	buildsystem/builder.h (33 lines)
A	M	buildsystem/builder.cpp (43 lines)
A	M	buildsystem/buildjob.h (27 lines)
A	M	buildsystem/buildjob.cpp (30 lines)
A	M	buildsystem/buildsystem.h (45 lines)
A	M	buildsystem/buildsystem.cpp (121 lines)
A	M	buildsystem/buildsystem.json (28 lines)
A	M	buildsystem/buildsystem.qrc (6 lines)
A	M	buildsystem/buildsystem.rc (9 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	12841	2a3a311		Mar 26 2017, 6:34 PM	★	★
Diff 2	12988	2a3a311		Mar 29 2017, 6:21 PM	★	★
Diff 3	15435	2a3a311		Jun 13 2017, 5:24 PM	★	★
Diff 4	15577	7d57641	R59:b4de161565e9bd0eb321e5bc4e432a371f431542	Jun 19 2017, 12:24 PM	★	★