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

Read packages source when library name is different from package name #469

Merged
merged 10 commits into from
Jan 21, 2016
Merged

Conversation

jaxx
Copy link
Contributor

@jaxx jaxx commented Jan 11, 2016

I tried to fix issue #449. Earlier Racer found package source when crate name was exactly the same as package name - for example if package name was rand and crate name was rand then it could navigate to the package source. If package name was rust-crypto, but crate name was crypto then it couldn't find nothing. So now I made few changes:

  1. Racer always iterates over all packages
  2. Then finds packages Cargo.toml files
  3. Reads package name from toml file (if lib section is present then this comes from lib, else from package section)

I'm kind of a Rust noob, so I think there's a lot room for optimizing this stuff (for example cache those packages in session etc). Also wanted to ask if there's a way to test this?

@jwilm
Copy link
Collaborator

jwilm commented Jan 11, 2016

Thanks for working on that issue!

It would be nice to not iterate over all packages for each request, but I don't think racer is really set up for caching this sort of thing right now.

For testing it, could we add a subcrate that is included as a dev-dependency? You would be able to import the crate as usual in the test module. For example, create a package named "test_fixtures", set the lib.name to "fixtures", and import via extern crate fixtures; in the tests.

@jaxx
Copy link
Contributor Author

jaxx commented Jan 11, 2016

I missed local packages support, I'm working on it :)

@phildawes
Copy link
Collaborator

Hi @jaxx, Hi @jwilm
I wonder, how does cargo handle this? Does it iterate all the packages to find the one with the matching name?

@jaxx
Copy link
Contributor Author

jaxx commented Jan 12, 2016

I think in cargo it's simpler, because this is one time operation there - packages are only iterated when compiling the project. I haven't looked into the source, but I think so. In Racer we need to know current state of the packages every time when user adds imports. Because I may add new dependency and not compile the project, but I want Racer to complete.

@jaxx
Copy link
Contributor Author

jaxx commented Jan 13, 2016

added few tests :)

phildawes and others added 2 commits January 16, 2016 08:28
Use defaults when lib.path or lib.name are not set when searching for main library crate
@jwilm
Copy link
Collaborator

jwilm commented Jan 16, 2016

@jaxx looks like this needs to be rebased

@jaxx
Copy link
Contributor Author

jaxx commented Jan 16, 2016

@jwilm done!

phildawes added a commit that referenced this pull request Jan 21, 2016
Read packages source when library name is different from package name
@phildawes phildawes merged commit e5b72c6 into racer-rust:master Jan 21, 2016
@phildawes
Copy link
Collaborator

Thanks v much for the @jaxx. Sorry it took so long for me to merge it

@Manishearth
Copy link
Contributor

Could you publish a new version for this? I want to test out the perf of YCM+racer on servo 😄

@phildawes
Copy link
Collaborator

Hi @Manishearth. I'm trying, but having problems with cargo package and the new test-fixtures. Hopefully soon...

@phildawes
Copy link
Collaborator

Published 1.2.1, but commented out the test-fixtures dev dependency so tests won't run. I think this is ok for cargo install though

@phildawes
Copy link
Collaborator

@Manishearth N.B. I think this patch iterates over the cargo packages, so it might be a bit slow - I haven't looked at profiling or optimizing it. Let me know what happens!

@Manishearth
Copy link
Contributor

Yeah, that's what I was concerned about the first time I installed YCM+racerd, since I recall that previously (like half a year ago) racer on servo used to be really slow. I wonder if racerd/racer can cache package names.

@jwilm
Copy link
Collaborator

jwilm commented Jan 21, 2016

I was thinking about more aggressive caching since this code introduces more O(n) searching for each request. The FileCache is kept alive by racerd between requests. This primarily saves disk I/O time. The Session object passed into Racer for each request is thrown out. This has been OK so far since the Session is mostly just a wrapper on the FileCache. The first stage of caching would be adding a package lookup cache to Session. This would only require changes to racer, and could increase performance within a single request. After that, we can look at keeping the cache alive in racerd.

@Manishearth if you don't want to wait for a package to be published, you can always modify your racerd locally and set g:ycm_racerd_binary_path (or whatever the Sublime equivalent is) to point to your custom built racerd binary.

@Manishearth
Copy link
Contributor

Nah, I just did a cargo update -p racer in the ycmd copy of racerd. I can also create a local clone of racer and use a path override to point racerd to it, but it's cleaner when things are published.

This update causes a crash, though: jwilm/racerd#20

phildawes added a commit that referenced this pull request Jan 22, 2016
fix when there's no dependencies in cargo file
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.

4 participants