Add working directory to clang parser.
ClosedPublic

Authored by bungeman on Jul 1 2019, 7:05 PM.

Details

Summary

This sets the working directory of the clang parser similarly to the way
the build directory is passed to the builder. This allows the parser to
correctly resolve any relative paths in the extra build arguments. This
means relative paths are resolved relative to the build directory (as
they would be resolved by a builder).

Test Plan

In the current test setup it doesn't seem the TestProject allows a TestBuildSystemManager, so it seems like quite a bit of work to create a test for this. (As a result I assume IBuildSystemManager integration isn't well tested in general right now.) If this needs a test, let me know if adding a TestBuildSystemManager is the right way to go.

Diff Detail

Repository
R32 KDevelop
Branch
working_directory (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13827
Build 13845: arc lint + arc unit
bungeman created this revision.Jul 1 2019, 7:05 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptJul 1 2019, 7:05 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
bungeman requested review of this revision.Jul 1 2019, 7:05 PM
bungeman updated this revision to Diff 61021.Jul 2 2019, 6:10 PM

Upload from arc to get everything filled out.

aaronpuchert added inline comments.
plugins/clang/duchain/parsesession.cpp
287–288

Is the build directory necessarily the working directory for compilation?

bungeman added inline comments.Jul 3 2019, 3:06 PM
plugins/clang/duchain/parsesession.cpp
287–288

No, it is the working directory for the builder for the given project item. Technically, this is not necessarily the directory from which the builder will actually invoke the compiler (the build system could do some lookup, change directories, and run the compiler from there; e.g. recursive make setups could do something like that in theory). To get a better directory, it may be possible to add another method to the IBuildSystemManager like parserDirectory(ProjectBaseItem*) to allow the build system manager to give even more accurate information (to distinguish between the directory the builder is invoked and the directory in which the parser / compiler is invoked).

That being said, the current cwd of the parser is the cwd of the kdevelop process (which is essentially random, so fixing this shouldn't break anything, unless someone is relying on the directory from which they launch kdevelop having an effect on how the parser resolves relative paths). Also, even the custommake plugin seems to try to make this sort of thing work as well as it can. However this does immediately fix an issue I am having, points out the need for this sort of information, and adds the framework for providing it.

That being said, maybe that means this should be called the parseDirectory instead of build(er)Directory.

aaronpuchert added inline comments.Jul 3 2019, 11:18 PM
plugins/clang/duchain/parsesession.cpp
287–288

Makes sense. This might not be perfect, but I agree it could improve some situations. We can do something more sophisticated should the need arise. I have also had cases where I needed to add include directories manually, perhaps because they were relative.

That being said, maybe that means this should be called the parseDirectory instead of build(er)Directory.

I would go with workingDirectory. Since the class is called ClangParsingEnvironment, the parse should be implied.

bungeman updated this revision to Diff 61548.Jul 10 2019, 8:21 PM

Change to workingDirectory, add comment.

This revision is now accepted and ready to land.Jul 10 2019, 10:26 PM
This revision was automatically updated to reflect the committed changes.