[READY] Add an option to make relative paths in flags from extra conf absolute

[micbou]

Since PR #680, all the infrastructure is here to convert any relative paths in the list of flags from the .ycm_extra_conf.py file to absolute ones. We just need to use the _MakeRelativePathsInFlagsAbsolute function.

This PR adds a new optional key working_directory to the dictionary returned by the FlagsForFile function in the .ycm_extra_conf.py file. This option allows specifying the working directory i.e. the directory for which the paths in the list of flags are relative to.

Update YCM's own .ycm_extra_conf.py file to reflect this change.

---

This change is 

[micbou]

Looks like Travis recently added the bin folder in the home directory. Linux builds are now failing with this error:

mkdir: cannot create directory ‘/home/travis/bin’: File exists

We need to merge PR #793 first to add the -p flag to the mkdir command in our Travis script. I can also create a separate PR for that.

---

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[codecov-io]

Codecov Report

Merging #795 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #795      +/-   ##
==========================================
+ Coverage   94.75%   94.79%   +0.03%     
==========================================
  Files          79       79              
  Lines        5317     5375      +58     
  Branches      170      170              
==========================================
+ Hits         5038     5095      +57     
- Misses        232      233       +1     
  Partials       47       47

[puremourning]

I worry that this is possibly a breaking change by default. By manipulating the paths when the new flag is not specified we are introducing silent change in behaviour, which I worry will break existing (working) installations. I am fairly certain that this would break my setup at work for example, where the relative nature of the include paths is accommodated in the make system integration.

---

Review status: 3 of 4 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[micbou]

This only affects relative paths. Are you passing relative paths to libclang? What's the expected behavior in that case? Seems undefined to me.

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[puremourning]

I'm pretty sure we pass -I. and it is interpreted relative to the TU. Memory could be off in that. Passing relative paths to a compiler is pretty normal behaviour. I suppose I t might be interpreted relative to the wd of ycmd, but (especially in the case of a global extra conf), I can't see a good justification for assuming that the YCM extra conf path as the root of such relative paths.

The relative paths in a standard cc invocation are interpreted relative to the wd of the compiler when invoked which is (let's say) typically the path of the cc file. In the compilation database case, we set the calls relative to the compiler wd entry in the database, not relative to the compile commands file.

Ofc every project is different, but that's why I'm nervous about making assumptions about users' project structure or paradigms.

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[Valloric]

I think @puremourning is right; this would break some people. We could perhaps do this without the new default of always resolving relative paths; we'd only do it when working_directory is provided.

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[micbou]

I'm pretty sure we pass -I. and it is interpreted relative to the TU.

Seems really convoluted to write include paths in its .ycm_extra_conf.py relative to the TU, i.e. the currently edited file.

I can't see a good justification for assuming that the YCM extra conf path as the root of such relative paths.

Because that's natural. When writing relative paths in a file, users expect them to be relative to this file.

I think @puremourning is right; this would break some people. We could perhaps do this without the new default of always resolving relative paths; we'd only do it when working_directory is provided.

This defeats the purpose of this PR which is helping users writing .ycm_extra_conf.py files, because they won't know about this option. They will write relative paths in their .ycm_extra_conf.py files, of course relative to the .ycm_extra_conf.py file, and it won't work.

I'd also like to point out that, in addition to being less than obvious (what is the actual working directory?), the current behavior for relative paths is undocumented. Do we want to go as far as not breaking backward compatibility for undocumented and obscure behavior?

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[puremourning]

The use case in question is where the flags are not actually writen into the .ycm_extra_conf.py, but the python within shells out to the make system to get the flags (in our case, effectively make COMPILER=clang show_CXXFLAGS). This returns the compile flags that would be passed for file_name when using the normal make system, rather than somehow repeating that logic within the extra conf file. Pretty sure we're not the only people which do this sort of thing, and it's essentially how the compile_commands.json works.

In any case, the current behaviour actually relative to the ycmd wd:

Given the following file structure:

$> find .
.
./test.h
./path
./path/test.h
./path/test.cc
./.ycm_extra_conf.py

with:

./test.h = int test;
./path/test.h = char test;
./path/test.cc =

#include <test.h>
#include <stdio.h>

int main()
{
    printf( "%c", test );
}

and .ycm_extra_conf.py:

def FlagsForFile( file_name, **kwargs ):
    return { 
        'flags': [
            '-x', 'c++',
            '-I.'
        ]
    }

• With Vim wd = ./path: :YcmCompleter GetType on test returns char (not int).
• With Vim wd = ./: YcmCompleter GetType on test returns int.

I confess that it (currently) being relative to ycmd working directory is arbitrary. However, I'm still not convinced that making it relative to the .ycm_extra_conf.py seems any less wrong. Again, in particular, when using a global ycm_extra_conf.py (which would be in any random dir).

If we were to pick a "default", I would suggest making it relative to the dirname of the file_name. Though this again is somewhat of a breaking change.

To be clear: I'm totally up for adding the working_dir item to the dict. This is a good move and I will likely implement it here as os.path.dirname( file_name ), but I'm still worried that forcefully changing the paths to an (arbitrarily) different default, is just causing potentially avoidable headaches in the (perhaps unlikely) case that people have become accustomed to this behaviour. I think we can say in our docs that the default is ycmd working dir, as that's what it currently is.

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[micbou]

You convinced me. I'll update the PR to only make the paths absolute when working_directory is specified (is working_dir a better name?). That's a little unfortunate for new users. I think they would expect paths to be relative to a local .ycm_extra_conf.py by default (as you said, this doesn't make sense for the global extra conf). But we can't break users that rely on the current behavior.

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[puremourning]

Maybe we should straight up call a spoon a spoon and call it paths_relative_to as we aren't actually wetting a working dir?

[Valloric]

Not at all against a clearer name than working_dir; we could call it include_paths_relative_to_dir and remove all confusion.

This defeats the purpose of this PR which is helping users writing .ycm_extra_conf.py files, because they won't know about this option.

I'd say the proportion of users who learn how to write an extra conf file by taking ycmd's (which we link to from the docs) and then change it to suit them is >95%. :D

So if we use it, they'll know about the option.

---

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[micbou]

Renamed the option to include_paths_relative_to_dir and updated the .ycm_extra_conf.py file in the examples folder too. PR is ready.

---

Reviewed 4 of 4 files at r3, 4 of 4 files at r4, 1 of 1 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[vheon]

Reviewed 2 of 4 files at r4, 1 of 1 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[Valloric]

Thanks!

---

Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[bstaletic]

Seems to work as expected. Thanks. Seeing that @puremourning was the last one asking questions, I'll let him trigger @zzbot.

---

Reviewed 2 of 4 files at r4, 1 of 1 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[puremourning]

Thanks!

@zzbot r+

---

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[zzbot]

📌 Commit 0f3aa20 has been approved by puremourning

[zzbot]

⌛ Testing commit 0f3aa20 with merge ccbaf71...

[zzbot]

☀️ Test successful - status-travis
Approved by: puremourning
Pushing ccbaf71 to master...