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

codeintel: add root to rust autoindex inferrence #29987

Closed
wants to merge 2 commits into from

Conversation

Strum355
Copy link
Member

Address issues found for this auto-index job. There is the "danger" that it only catches the demo projects, but this is a step in the right direction

@Strum355 Strum355 added team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) auto-index labels Jan 20, 2022
@Strum355 Strum355 requested a review from a team January 20, 2022 17:49
@Strum355 Strum355 self-assigned this Jan 20, 2022
@cla-bot cla-bot bot added the cla-signed label Jan 20, 2022
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Jan 20, 2022

Notifying subscribers in CODENOTIFY files for diff c7f9aea...048726a.

Notify File(s)
@efritz lib/codeintel/autoindex/inference/rust.go
lib/codeintel/autoindex/inference/rust_test.go

@Strum355
Copy link
Member Author

@varungandhi-src I had to fix a test here but Im wondering why there was dir1 but the index job wasn't rooted at that.

@efritz
Copy link
Contributor

efritz commented Feb 10, 2022

Should we merge this?

@varungandhi-src
Copy link
Contributor

varungandhi-src commented Feb 10, 2022

I had to fix a test here but Im wondering why there was dir1 but the index job wasn't rooted at that.

Because I thought it didn't matter. rust-analyzer will find any nested Cargo.toml files and index the crates correctly. I've written a longer comment in https://github.com/sourcegraph/lsif-rust/blob/main/lsif-rust :

# rust-analyzer's path lookup looks in subdirectories by default. For example,
# in the general case, if you have multiple parallel workspaces, under say
# - A/B/C with crates X1, X2
# - A/B/D with crates Y1, Y2
# and you run rust-analyzer with A/ as the project path, it will pick up all
# crates (X1, X2, Y1 and Y2). So it is enough to run rust-analyzer once,
# instead of once per workspace, or once per crate.

Taking a look at the failed job...

@varungandhi-src
Copy link
Contributor

varungandhi-src commented Feb 10, 2022

I think this patch will actually create a regression, because the loop exits on the first match. So it won't emit multiple index jobs for the https://github.com/Picovoice/rhino repo, just one at the most. Also, I don't think it's just a matter of fixing the early exit because we should be able to handle workspaces... (naively picking up all Cargo.toml files will end up indexing crates twice.)

I think we should land a fix in lsif-rust itself, which can handle the path checking logic and keep this simple. I can submit a patch for that.

@varungandhi-src
Copy link
Contributor

Oh sorry, I just realized this patch is ~21 days old. Somehow I just missed it. My bad. 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-index cla-signed team/graph Graph Team (previously Code Intel/Language Tools/Language Platform)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants