Racer now uses rustup to find the stdlib source code even when used as a library. #799

golddranks · 2017-09-23T17:02:35Z

Refactored the stdlib source code search a bit: It now stores the path in a lazy_static, and doesn't load it directly from an env var at usage sites. When initializing the lazy_static, it checks both the RUST_SRC_PATH env var and rustup's rust-src component. It used use rustup only when using Racer as a binary. Now it does it even when used as a library.

This allows using RLS without setting RUST_SRC_PATH; it suffices that rustup is installed. Before, it just used to crash if RUST_SRC_PATH wasn't set.

Also got rid of the warnings: converted a few \\\ doc comments that weren't actual item doc comments to \\ normal comments, and got rid of the non-used extern crates that were likely left from when the binary and library were not separated.

…a library.

golddranks · 2017-09-23T17:03:54Z

This helps with, although doesn't eliminate this: #751

golddranks · 2017-09-24T06:20:53Z

Oops, tests fail because the env_logger was still needed in tests. Fixing!

TedDriggs

Added comments; most are minor.

Is it possible to include a test for this? I know that the fixtures directory is pretty out-of-date, so if we can't then that's fine.

TedDriggs · 2017-09-28T18:06:39Z

src/racer/config.rs

@@ -0,0 +1,23 @@
+use std::path::PathBuf;


Having a generic config.rs makes me suspicious; it seems likely that it'll become a parking lot for a bunch of unrelated items. Can you make the name for this more specific to file paths?

Yes, I can; however, I originally thought that it would be actually good to have a centralised module for reading any needed configuration from the environment and keeping the state. Do you think it would make sense to move this to nameres.rs where RUST_SRC_PATHS is used?

I'm personally a fan of smaller, purpose-oriented modules, so I'd suggest one that only contained this and have that be used by nameres. However, I'm fine with it living in nameres.rs directly for the time-being.

All right, I'll refactor that.

TedDriggs · 2017-09-28T18:08:11Z

src/racer/core.rs

-            /// Test if the **path expression** starts with `::`, in which case the path
-            /// should be checked against the global namespace rather than the items currently
-            /// in scope.
+            // Test if the **path expression** starts with `::`, in which case the path


Thanks for cleaning these up; it makes me sad to lose the ability to add doc comments to locals and have them show up in completions, but the rustdoc team has spoken.

Did they actually consider that use case? It might be that they just missed it, and deprecated this as an useless thing.

They considered it; I asked them about it, which was what prompted the discussion which ended in deprecation.

TedDriggs · 2017-09-28T18:08:43Z

src/racer/nameres.rs

@@ -11,13 +11,9 @@ use matchers::find_doc;
 use cargo;
 use std::path::{Path, PathBuf};
 use std::{self, vec};
+use std::iter;


Can you combine this with the use on the previous line?

TedDriggs · 2017-09-28T18:10:02Z

src/racer/nameres.rs

-    let srcpaths = match std::env::var("RUST_SRC_PATH") {
-        Ok(paths) => paths,
-        Err(_) => return out.into_iter()
+    if config::RUST_SRC_PATHS.is_empty() {


This change makes the comment above inaccurate; can you please update it?

golddranks · 2017-09-28T23:24:06Z

What is the purpose of the fixtures directory?

golddranks · 2017-10-02T02:24:53Z

Ping @TedDriggs , in case you didn't notice my updates and question.

TedDriggs · 2017-10-03T19:46:10Z

What is the purpose of the fixtures directory?

I actually don't know, but I got the impression that it was supposed to do something around environment tests like this.

jwilm · 2017-10-03T19:55:45Z

The fixtures directory has projects for tests. It allows us to test very specific setups; although, it hasn't gotten much use outside of 1-2 tests.

golddranks · 2017-10-04T03:29:49Z

Since there was a TODO comment that indicated that a more thorough refactoring might be good, I went ahead and did that.

It now resolves to a single path using multiple sources (the env var, rustup and default path list) that is verified to contain the stdlib source.

Also added tests.

golddranks · 2017-10-04T03:31:58Z

I'd very much appreciate if, when this gets pulled in, a new version of Racer would be released; updating RLS to use the new version, it would make RLS setup one step easier.

golddranks · 2017-10-04T07:24:53Z

The Windows build fails the test that uses rustup to get the path. Is rustup present on Windows? And does it work similarly than on macOS, which is the system I'm using?

TedDriggs · 2017-10-04T22:46:17Z

The Windows build seems to work on all but one architecture, so I don't think it's a problem with the AppVeyor config.

golddranks · 2017-10-05T13:10:36Z

All right, all requested changes are done (Although Github claims otherwise) and tests are passing.

golddranks · 2017-10-10T03:36:52Z

Friendly ping to @TedDriggs . This PR is waiting for a (hopefylly) final review for merge.

bkchr · 2017-10-10T20:20:57Z

src/racer/util.rs

+    }
+    assert!(get_rust_src_path().is_ok());
+
+    match original {


Nitpick: You could create a struct, that stores the original RUST_SRC_PATH and implement Drop. If drop is called, you would reset to the original env var. This could reduce some code duplication in the tests :)

TedDriggs

LGTM. If @jwilm is happy then we can go ahead and merge.

golddranks · 2017-10-16T07:56:34Z

Ping @jwilm , is there anything I can do to make this go forward?

golddranks · 2017-10-30T04:09:06Z

@TedDriggs is there any other maintainers that could approve of this?

Racer now uses rustup to find the rust source code even when used as …

6e62c2b

…a library.

golddranks mentioned this pull request Sep 23, 2017

Adding support for Sublime Text 3 rust-lang/rls#214

Closed

Added extern crate env_logger to the test module.

93d3978

golddranks force-pushed the master branch from 1e5a99a to 93d3978 Compare September 24, 2017 06:23

urschrei mentioned this pull request Sep 26, 2017

RLS support thread rust-lang/rust-enhanced#210

Open

TedDriggs requested changes Sep 28, 2017

View reviewed changes

Addressed two comments.

c5ba58f

golddranks force-pushed the master branch from 1707b42 to c5ba58f Compare September 28, 2017 22:22

The RUST_SRC_PATHS config setting now lives in nameres.rs

853245a

golddranks force-pushed the master branch from 18887c8 to 853245a Compare October 4, 2017 01:55

Refactored the Rust stdlib source path config more thoroughly.

621364c

golddranks added 2 commits October 5, 2017 19:27

Added debug printing to explore what goes wrong in the test environment.

d886c61

Added a mutex so that the tests don't mess up each other's environment.

146d6ed

bkchr reviewed Oct 10, 2017

View reviewed changes

TedDriggs approved these changes Oct 10, 2017

View reviewed changes

jwilm merged commit cd80078 into racer-rust:master Oct 30, 2017

apiraino mentioned this pull request May 31, 2018

emacs-racer seems to have a different idea about RUST_SRC_PATH than the system racer-rust/emacs-racer#93

Open

kngwyu mentioned this pull request Jun 6, 2018

Don't panic about RUST_SRC_PATH #751

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Racer now uses rustup to find the stdlib source code even when used as a library. #799

Racer now uses rustup to find the stdlib source code even when used as a library. #799

golddranks commented Sep 23, 2017 •

edited

Loading

golddranks commented Sep 23, 2017

golddranks commented Sep 24, 2017

TedDriggs left a comment

TedDriggs Sep 28, 2017

golddranks Sep 28, 2017

TedDriggs Oct 3, 2017

golddranks Oct 4, 2017

TedDriggs Sep 28, 2017

golddranks Sep 28, 2017

TedDriggs Oct 3, 2017

TedDriggs Sep 28, 2017

golddranks Sep 28, 2017

TedDriggs Sep 28, 2017

golddranks Sep 28, 2017

golddranks commented Sep 28, 2017

golddranks commented Oct 2, 2017

TedDriggs commented Oct 3, 2017

jwilm commented Oct 3, 2017

golddranks commented Oct 4, 2017

golddranks commented Oct 4, 2017

golddranks commented Oct 4, 2017

TedDriggs commented Oct 4, 2017

golddranks commented Oct 5, 2017

golddranks commented Oct 10, 2017

bkchr Oct 10, 2017

TedDriggs left a comment

golddranks commented Oct 16, 2017

golddranks commented Oct 30, 2017

Racer now uses rustup to find the stdlib source code even when used as a library. #799

Racer now uses rustup to find the stdlib source code even when used as a library. #799

Conversation

golddranks commented Sep 23, 2017 • edited Loading

golddranks commented Sep 23, 2017

golddranks commented Sep 24, 2017

TedDriggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

golddranks commented Sep 28, 2017

golddranks commented Oct 2, 2017

TedDriggs commented Oct 3, 2017

jwilm commented Oct 3, 2017

golddranks commented Oct 4, 2017

golddranks commented Oct 4, 2017

golddranks commented Oct 4, 2017

TedDriggs commented Oct 4, 2017

golddranks commented Oct 5, 2017

golddranks commented Oct 10, 2017

Choose a reason for hiding this comment

TedDriggs left a comment

Choose a reason for hiding this comment

golddranks commented Oct 16, 2017

golddranks commented Oct 30, 2017

golddranks commented Sep 23, 2017 •

edited

Loading