[READY] Add checks to ycm_core library when starting ycmd

[micbou]

Do the following checks on the ycm_core library when starting ycmd:

• missing library;
• compiled with Python 2 but imported with Python 3;
• compiled with Python 3 but imported with Python 2;
• outdated library.

When one of these errors is encountered, exit with a non-zero status code specific to the error. These status codes will be used in the YCM client to display an appropriate error message to the user. In addition, starting a separate process to check the core version will not be needed anymore. This should slighty improve the Vim startup time. See issue ycm-core/YouCompleteMe#2085.

---

This change is

[coveralls]

Coverage increased (+0.1%) to 84.821% when pulling 964d969 on micbou:check-core into a3c41b4 on Valloric:master.

[coveralls]

Coverage increased (+0.2%) to 84.904% when pulling 694e5ae on micbou:check-core into a3c41b4 on Valloric:master.

[Valloric]

This is a pretty fantastic change! I'm not a fan of us parsing import error messages, but I don't see a better way of doing it so we'll live.

@abingham @ptrv @mawww @Qusic Note that this is a breaking change to clients because it deletes check_core_version.py. See the comments in CompatibleWithCurrentCore() in server_utils.py in this PR for a list of ycmd process return codes that are replacing that file.

@micbou This PR should probably change the README as well to document this. Shouldn't be much beyond the comment you have in the code transplanted into a new section in the README.

---

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/server_utils.py, line 40 [r2] (raw file):
This message is likely to be encountered by people completely unfamiliar with YCM so it should be more detailed. For instance, it should refer to the installation guide, much like the old message in handlers.py.

In general, I try to avoid emitting error messages that don't indicate a clear way towards solving the problem. "You need to compile it" may be clear to me and you, but the more direct we are, the fewer issue reports we'll get.

---

ycmd/tests/server_utils_test.py, line 66 [r2] (raw file):
This 0, 1, 0 construction appears too magic. Can we refactor this a bit so that it's more clear to the reader what's going on?

I think we used a different idiom for asserting calls on mocks in other tests, no?

---

Comments from Reviewable

[puremourning]

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/server_utils.py, line 71 [r2] (raw file):
Drive-by comment: can we define the return codes with symbolic names? They can then also be imported by the client to avoid magic numbers.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.2%) to 84.929% when pulling 0a7142c on micbou:check-core into a3c41b4 on Valloric:master.

[coveralls]

Coverage increased (+0.2%) to 84.929% when pulling 6b771b5 on micbou:check-core into a3c41b4 on Valloric:master.

[coveralls]

Coverage increased (+0.2%) to 84.929% when pulling 1230ca1 on micbou:check-core into a3c41b4 on Valloric:master.

[micbou]

I added a section to the README and I assigned a specific exit code for the "unexpected error" case instead of using the general exit code 1 so that we can handle it in the clients.

---

Reviewed 5 of 6 files at r1, 1 of 1 files at r2, 2 of 3 files at r4, 2 of 2 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/server_utils.py, line 40 [r2] (raw file):
I updated the messages.

---

ycmd/server_utils.py, line 71 [r2] (raw file):
Done. Since the symbolic names can only be used by python clients, the README only talks about the hardcoded values.

---

ycmd/tests/server_utils_test.py, line 66 [r2] (raw file):
I added the log_level parameter to the tests so that we can easily use the assert_called_once_with method. I think it is clearer this way.

---

Comments from Reviewable

[Valloric]

---

Reviewed 4 of 6 files at r1, 2 of 2 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

ycmd/server_utils.py, line 71 [r2] (raw file):
The documentation of what the states mean should now probably be on the constants themselves instead of here.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.2%) to 84.929% when pulling 9eba7c2 on micbou:check-core into a3c41b4 on Valloric:master.

[micbou]

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

ycmd/server_utils.py, line 71 [r2] (raw file):
Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.2%) to 84.929% when pulling bb062da on micbou:check-core into a3c41b4 on Valloric:master.

[Valloric]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

---

ycmd/server_utils.py, line 51 [r7] (raw file):
Why the doc comment? Why not just lines prefixed with #?

---

Comments from Reviewable

[vheon]

Reviewed 4 of 6 files at r1, 1 of 2 files at r5, 1 of 1 files at r6, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.

---

ycmd/tests/server_utils_test.py, line 73 [r7] (raw file):
If we inline the implementation for this case we could simplify the implementation of RunCompatibleWithCurrentCore and improve its readability. If we do that maybe we can also rename it like RunNonCompatibleWithCurrentCore since we would be using it only for the non-compatible cases. What do you think?

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.2%) to 84.929% when pulling 267a365 on micbou:check-core into 89e558f on Valloric:master.

[coveralls]

Coverage increased (+0.2%) to 84.929% when pulling 267a365 on micbou:check-core into 89e558f on Valloric:master.

[micbou]

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/server_utils.py, line 51 [r7] (raw file):
Trying to be smart I guess. I changed it.

---

ycmd/tests/server_utils_test.py, line 73 [r7] (raw file):
We can't really simplify the RunCompatibleWithCurrentCore function because there are other tests not using the import_error key: the two CompatibleWithCurrentCore_Outdated tests.

---

Comments from Reviewable

[vheon]

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

---

ycmd/tests/server_utils_test.py, line 73 [r7] (raw file):
I've overlooked that apparently. Is that to me it looks like we're trying to squeeze three kind of test into one only because they share 1 line of code and the other one a similar. But I guess is just me :)

---

Comments from Reviewable

[micbou]

I made some changes to the tests (they are now more robust and more readable). I am marking the PR as ready. Tell me if there is something that should be improved before we merge this.

---

Reviewed 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

ycmd/tests/server_utils_test.py, line 73 [r7] (raw file):
So I followed your advice of not merging all the tests into one and I think you were right, it is better now.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.2%) to 84.929% when pulling 85d5f6b on micbou:check-core into f3dd8fd on Valloric:master.

[coveralls]

Coverage increased (+0.2%) to 84.929% when pulling 85d5f6b on micbou:check-core into f3dd8fd on Valloric:master.

[Valloric]

I continue to be satisfied with this. :)

---

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[vheon]

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/tests/server_utils_test.py, line 73 [r7] (raw file):
I like it better now 👍

---

ycmd/tests/server_utils_test.py, line 150 [r9] (raw file):
Should we use the variable names instead of the bare numbers like for the exit status? (I'm saying this here because if it is the case then we should add a comment to the top of this method to explain why there are 10 and 11 as return_value for some mocked function.

---

Comments from Reviewable

[micbou]

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

---

ycmd/tests/server_utils_test.py, line 150 [r9] (raw file):
It is better to use the numbers in tests so that we also check the correspondance between these numbers and the symbolic names.

---

Comments from Reviewable

[vheon]

Then I believe that this is ready to go. @homu r=Valloric

---

Reviewed 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[homu]

📌 Commit 85d5f6b has been approved by Valloric

[homu]

⚡ Test exempted - status