Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[READY] Fix Python 2 support and add Python 3 support on Windows #365

Merged
merged 6 commits into from
Feb 15, 2016

Conversation

micbou
Copy link
Collaborator

@micbou micbou commented Feb 14, 2016

Python 2 support

We cannot pass an environment variable containing unicode literals to Popen on Windows and Python2. Instead of converting all environment variables to native strings, we only convert the ones we use.

Python 3 support

We update the requests submodule to fix a compatibility error between requests and Python 3.5.

Fix from PR #211 is only needed for Python 2.

In build.py, we prepend the folder of the ycm_core_tests executable to the PATH instead of replacing it so that the executable is able to find the python35.dll library.

We don't reuse Omnisharp instances in C♯ tests on Windows and Python 3 because of the following random error occuring when running those tests:

OSError: [WinError 6] The handle is invalid

AppVeyor configuration

We append the -DUSE_DEV_FLAGS=ON option to EXTRA_CMAKE_ARGS environment variable in run_tests.py script in order to use EXTRA_CMAKE_ARGS when running tests. We need it on AppVeyor to tell CMake the correct path to the Python 3 library according to architecture. Otherwise, it always pick the 64 bits one. I can't reproduce this issue on my machine so I consider it is specific to AppVeyor.

We only do one run for MSVC 11, MSVC 12, and Python 2.7. This reduces the number of runs from 6 to 5.

Review on Reviewable

@Valloric
Copy link
Member

Thanks for making sure we continue to work well on Windows! :) Without you, it would probably bitrot.

:lgtm: with minor comments.


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


build.py, line 3 [r1] (raw file):
Not having this needs a comment explaining why it's missing otherwise someone will add it again in the future.


build.py, line 283 [r1] (raw file):
Needs comment.


Comments from the review on Reviewable.io

Calling subprocess on Windows and Py2 with environment variable
containing unicode literals will raise a TypeError. To prevent this,
we convert the variable and its value to native strings.
Update requests submodule to fix the following error in requests with
Python 3.5:

  TypeError: a bytes-like object is required, not 'str'
Fix issue finding the python35.dll library.
Reusing Omnisharp instances on Windows and Python 3 randomly raises
the error "OSError: [WinError 6] The handle is invalid" in tests.
@micbou
Copy link
Collaborator Author

micbou commented Feb 15, 2016

Reviewed 8 of 9 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


build.py, line 3 [r1] (raw file):
Done.


build.py, line 283 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@Valloric
Copy link
Member

Thanks!

Valloric added a commit that referenced this pull request Feb 15, 2016
[READY] Fix Python 2 support and add Python 3 support on Windows
@Valloric Valloric merged commit db76597 into ycm-core:more-py3 Feb 15, 2016
@micbou micbou deleted the windows-more-py3 branch February 20, 2016 13:28
homu added a commit that referenced this pull request Mar 6, 2016
[RFC] Rewrite tests to reuse server state

This PR rewrites tests from completers using a server by separating them into two classes:
 - isolated tests: each test use its own server instance;
 - persistent tests: use the same server instance.

We use classes because it provides a clean way to do this (no global variables needed).

This PR also simplifies the C♯ tests by adding filepaths used to start the server to a list and stopping the server for each filepath in the list at the end of the tests. With this approach, we don't need to expose additional functions in the C♯ completer (`GetOmnisharpPort` and `SetOmnisharpPort` are not needed). Furthermore, we don't have the issue with persistent tests (introduced by PR #339) on Windows and Python 3. See PR #365 for details.

And now, the interesting part; the execution times on different CI configurations before and after the changes:

Configuration | Before (s) | After (s)
------------- | -----------| ---------
AppVeyor Python 2.7 | 58.4 | 28.8
AppVeyor Python 3.5 | 161.1 | 32.8
Travis Linux | 49.0 | 32.6
Travis OS X | 59.4 | 42.8

Closes #338.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/409)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants