Differential D11921

CMake Server: Reuse existing build directory path when available
ClosedPublic
Actions

Authored by mwolff on Apr 4 2018, 11:06 AM.

Details

Reviewers
rjvbb
Group Reviewers
KDevelop
Commits
R32:e9b0a34047dc: CMake Server: Reuse existing build directory path when available
Summary

When the build dir is accessible via multiple (symlinked) paths,
we should reuse whatever the current CMakeCache is using. This
ensures that we don't trigger a needless cache invalidation leading
to a full project rebuild. E.g. I have:

/home/milian/projects/kf5/src/frameworks/kdesu

kdesrc-build will build this in:
/home/milian/projects/kf5/build-dbg/frameworks/kdesu

To enable quick build dir selection by kdevelop, I also
created this symlink:
/home/milian/projects/kf5/build -> /home/milian/projects/kf5/build-dbg

Thus KDevelop is running the cmake server configuration job with
this path instead of the one chosen by kdesrc-build above:
/home/milian/projects/kf5/build/frameworks/kdesu

In the end, this lead to me having to recompile every project after
I ran kdesrc-build and opening the project in kdevelop...

This patch looks up the existing CMAKE_CACHEFILE_DIR variable
and reuses that if available. I.e. now KDevelop will actually use
/home/milian/projects/kf5/build-dbg/frameworks/kdesu automatically,
and I don't need to recompile everything all the time.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mwolff created this revision.Apr 4 2018, 11:06 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptApr 4 2018, 11:06 AM
mwolff requested review of this revision.Apr 4 2018, 11:06 AM
mwolff updated this revision to Diff 31288.

remove unrelated changes (oh how I hate arc)

mwolff added a comment.Apr 4 2018, 11:08 AM

@rjvbb does this solve issues you are having and trying to workaround with https://phabricator.kde.org/D7930 ?

apol added a subscriber: apol.Apr 4 2018, 11:28 AM

Makes sense to me. +1

mwolff added subscribers: brauch, kfunk.Apr 4 2018, 11:42 AM

should it go to master, or 5.2? @brauch, @kfunk? I'd like others to test this too first before landing in 5.2, since I've never used CMAKE_CACHEFILE_DIR before and am not sure if it's OK to use it like this.

rjvbb added a comment.Apr 4 2018, 12:03 PM

I can try to test with 5.2.1, but not before tonight.

kfunk added a comment.Apr 4 2018, 2:06 PM

Patch looks good to me, I think after some successful testing it should go to 5.2 branch.

rjvbb added a comment.Apr 4 2018, 4:35 PM
(oh how I hate arc)

(Guess why I use it almost exclusively via the Purpose plugin to upload "raw diffs" created in KDevelop's Patch Review toolview. I just have to remember to trigger the (my) hidden widget to select an "infinite" number of context lines for the patch so Phab will have context to show.)

brauch added a comment.Apr 4 2018, 4:44 PM

Thank you for fixing this! 5.2 sounds ok to me, but let's not put it into the release we were planning for the next few days (i.e. not in 5.2.2), or should we?

rjvbb accepted this revision.Apr 4 2018, 5:13 PM
@rjvbb does this solve issues you are having and trying to workaround with https://phabricator.kde.org/D7930 ?

The problem I try to solve with D7930 goes beyond having to rebuild everything every time: it is about code parsing failing when auto-generated (.moc) files use relative include paths. This can even fail during an actual build if the symlinked path doesn't have the same number of subdirectories relative to the root as the normalised path.

It looks like this patch should help with that issue, provided that the CMake stores the actual, normalised path in CMAKE_CACHEFILE_DIR, does it?

I'm not noticing any different with and without this patch applied, on Mac. Not really surprising as the system always normalises paths, so applications cannot obtain a PWD path that contains symlinks.

I've never used CMAKE_CACHEFILE_DIR before and am not sure if it's OK to use it like this.

According to the docs This [variable] is the same as CMAKE_BINARY_DIR and thus should be the same path as KDevelop's $build.dir setting. The change should be safe so I agree with Sven: apply this to 5.2 but after the 5.2.2 release. Or better yet, delay that release a couple of days so more people can test and (many) more users can benefit from the fix a lot sooner.

This revision is now accepted and ready to land.Apr 4 2018, 5:13 PM
mwolff added a comment.Apr 4 2018, 6:55 PM

I'll push this to master then and we can cherry pick it to 5.2 after you guys did the next release

thanks for the reviews everyone

Closed by commit R32:e9b0a34047dc: CMake Server: Reuse existing build directory path when available (authored by mwolff). · Explain WhyApr 4 2018, 6:56 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mplugins/cmake/cmakeserver.cpp (23 lines)

Diff 31313

View Options

plugins/cmake/cmakeserver.cpp

Loading...