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

Update Racerd #1212

Closed
wants to merge 6 commits into from
Closed

Update Racerd #1212

wants to merge 6 commits into from

Conversation

fandahao17
Copy link

@fandahao17 fandahao17 commented Mar 19, 2019

Hi there!
I know that ycmd is migrating rust completer to RLS, but the Racerd version that ycmd currently uses is too old (last update 2 years ago) and doesn't work well under recent Rust versions (most times no completion pops up after pressing ' . ', not recognizing some Rust 2018 features, etc.). So it's necessary to switch to the most recent Racerd version before we finally migrate to RLS.

Since starting from version 2.1, racer needs nightly rust, I also added "nightly" in build scripts, test scripts and the documentation.

But as was mentioned in the issue above, Racerd isn't under effective maintenance and its most recent version is still a little bit out of date. This pull request there tries to keep it up-to-date.


This change is Reviewable

@fandahao17
Copy link
Author

The test shows what I'm worried about. Even the most recent version of racerd can't compile under newest Rust compilers. This can be solved by updating its dependencies in the Cargo.toml file.

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good, except for the failing tests.
Here's the branch for RLS support https://github.com/micbou/ycmd/commits/rust-language-server
The problem is that tests on Windows are very flaky, if I remember @micbou's words correctly.

Reviewed 1 of 1 files at r1, 5 of 5 files at r2.
Reviewable status: 0 of 2 LGTMs obtained

@fandahao17
Copy link
Author

The tests failed because some of racerd's dependencies are too old. @jwilm Could you merge the PR to update Racerd so that we can provide better support for recent Rust versions?

@jwilm
Copy link
Contributor

jwilm commented Mar 20, 2019

Merged the PR you linked @fandahao17

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why you installed rust-src and set RUST_SRC_PATH, because it broke the tests.

Reviewed 4 of 4 files at r3.
Reviewable status: 0 of 2 LGTMs obtained

@fandahao17
Copy link
Author

Sorry for so many failed commits. I mistakenly thought the tests failed because of the lack of rust-src.However, I finally found that there may be some problems with our tests. As the error message indicates, the completion requests that were supposed to fail ended up getting a successful completion result. This may be caused by racer automatically detecting Rust sys root location ( discussed here and here, which was introduced after these tests were written. As a consequence, the 4 tests with

@patch( 'ycmd.completers.rust.rust_completer._GetRustSysroot',
        return_value = '/non/existing/rust/src/path' )

will not actually change the Rust sysroot value.
However, removing these tests doesn't seem like a good choice. But I haven't come up a better idea yet.

@bstaletic
Copy link
Collaborator

So... Appveyor failure is a flake. CircleCI I don't know what it is about, perhaps another flake.

However, removing these tests doesn't seem like a good choice. But I haven't come up a better idea yet.

Just removing the tests to get a passing build would be wrong. If I understand correctly, latest racerd doesn't need $RUST_SRC_PATH for basic completion and can even use rustup to figure things out. Sounds great. We should update the rust_completer.py to reflect these changes and then update the tests according to any new code we introduce.

@fandahao17
Copy link
Author

Sounds great. We should update the rust_completer.py to reflect these changes and then update the tests according to any new code we introduce.
Perhaps I'll give it a try.

@fandahao17 fandahao17 closed this Mar 21, 2019
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.

3 participants