Circular dependency in crates trips up rust-analyzer

[geigerzaehler]

The integration tests in librad depend on the librad-test crate which in turn depends on the librad library. While cargo can deal with this circular dependecy rust-analyzer cannot. For all imports in of librad in librad-test I see

[rust-analyzer unresolved-import] [E] unresolved import

To address this I’d suggest moving the integration tests from librad/tests to librad-test/tests.

[FintanH]

Would it be better to open a ticket on the rust-analyzer repo, since cargo can handle this just fine?

[geigerzaehler]

Would it be better to open a ticket on the rust-analyzer repo, since cargo can handle this just fine?

I’m working on posting an issue there. We should still consider fixing this here since we don’t know if and when this will be fixed.

[FintanH]

Is this only a problem for the integration tests or does it also happen in librad unit tests, for example, librad::peer using librad_test::roundtrip?

[kim]

librad-test was introduced as a way to have some common test utils which are usable across crate boundaries (ie. where #[cfg(test)] has no effect). Moving that code to the tests directory doesn't make sense thus, because it wouldn't be available to other crates. That is, unless this somehow became obsolete by #576

Afaiu, integration tests are treated as if they were their own crate, so I don't see how librad-test -> librad, librad/tests -> librad, librad/tests -> librad-test forms a circle, so I would consider this behaviour of rust-analyzer a bug.

More generally, I'm missing from the issue description why it is a bad thing that rust-analyzer is being tripped up, and why I should care.

[geigerzaehler]

Moving that code to the tests directory doesn't make sense thus, because it wouldn't be available to other crates. That is, unless this somehow became obsolete by #576

I wasn’t suggesting moving the code from librad-tests to librad/tests but the code from librad/tests to librad-tests/tests. I wasn’t aware though that the unit tests (which are not part of a different crate) also use librad-tests so this solution only works if all tests are separated.

I'm missing from the issue description why it is a bad thing that rust-analyzer is being tripped up, and why I should care.

You should care because it makes the life the developers using rust-analyzer easier if it actually works on the project and it makes it more likely that they will contribute if the tooling works.

[kim]

You should care because it makes the life the developers using rust-analyzer easier if it actually works on the project and it makes it more likely that they will contribute if the tooling works.

I don't use rust-analyzer, so I would have appreciated an explanation of what it is, and why it is a bad thing that it doesn't work. This is general good practice when reporting issues.

[geigerzaehler]

At its core, rust-analyzer is a library for semantic analysis of Rust code as it changes over time. This manual focuses on a specific usage of the library — running it as part of a server that implements the Language Server Protocol (LSP). The LSP allows various code editors, like VS Code, Emacs or Vim, to implement semantic features like completion or goto definition by talking to an external language server process.

https://rust-analyzer.github.io

If rust-analyzer doesn’t work it makes development for those using it more cumbersome than they are used to from other projects. More cumbersome means they can’t rely on all these nice LSP features.

It turns out that the cyclic dev-dependency librad -> librad-test -> librad also prevents us from publishing the crate (rust-lang/cargo#4242).

And here’s the rust-anylyzer issue, open since 2019: rust-lang/rust-analyzer#2414

[kim]

Following these links it looks like even rust-lang/rust triggers this. I'm sorry, but I have no interest in maintaining workarounds for broken tools, nor to accomodate for everyone's preferred development setup. The only reason I'm okay with a `default.nix` in this repo is that I trust @FintanH will fix it when it breaks. That being said, there were a lot of testing-related tickets submitted recently, where #594 has a link to a particularily noteworthy [article]. The interesting bit is the last sentence:

They add [lib] test = false to Cargo.toml, make all APIs they want to unit test public and have a single test crate for the whole workspace. This crates links everything and contains all the unit tests.

I think that this is the right way to do away with all the not-quite features of the tooling once and for all. It would also encourage to treat the test harness as a library and reduce duplication. [article]: https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html

[geigerzaehler]

They add [lib] test = false to Cargo.toml, make all APIs they want to unit test public and have a single test crate for the whole workspace. This crates links everything and contains all the unit tests.
I think that this is the right way to do away with all the not-quite features of the tooling once and for all. It would also encourage to treat the test harness as a library and reduce duplication.

That sounds like a reasonable approach and solution to this particular issue.

Happy to create an issue for this, hash this out and start working on it.

[FintanH]

The only reason I'm okay with a default.nix in this repo is that I trust @FintanH will fix it when
it breaks.

😇

[kim]

I suppose this works since #666?

[geigerzaehler]

Yes! Everything works fine now.