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

rust-analyze: only stick with the deps/save-analysis file, instead of all of them. #81

Closed
wants to merge 4 commits into from

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Jan 20, 2018

When building with crates.io, all the crate dependencies also get save-analysis
dirs.

We probably want to use them, but not quite yet.

@emilio
Copy link
Contributor Author

emilio commented Jan 20, 2018

This unblocks servo.

Copy link
Contributor

@staktrace staktrace left a comment

Choose a reason for hiding this comment

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

This looks fine to me, although the backslash seems weird. I would have expected quotes around "deps/save-analysis" to do the job.

It looks like on m-c all the analysis goes into deps/save-analysis so this shouldn't affect m-c at all.

emilio added 4 commits March 27, 2018 13:37
The Rust indexer is about to create stuff under __GENERATED__.
It's easier to figure out mismatches for rls-data, for example.
It's not clear the shell script accounts correctly for whitespace in the paths.

That being said it's not clear other scripts do so, and we have control over the
paths anyway.
@asutherland
Copy link
Contributor

@emilio Is there anything from this patch we still want noting that we may want to switch to using LSIF for rust (https://bugzilla.mozilla.org/show_bug.cgi?id=1761287) and that I think I addressed the generated files issues in other patches (#299 and #339 maybe based on blame)?

Note that the rust stuff is not super high on my personal priority list right now, but the JS LSIF stuff is (https://bugzilla.mozilla.org/show_bug.cgi?id=1740290), and there's a good chance of there being synergies there.

@emilio
Copy link
Contributor Author

emilio commented Mar 28, 2022

Yeah, let's close for now.

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