Closed geigerzaehler closed 3 years ago
Would it be better to open a ticket on the rust-analyzer
repo, since cargo can handle this just fine?
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.
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
?
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.
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.
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.
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 (https://github.com/rust-lang/cargo/issues/4242).
And here’s the rust-anylyzer
issue, open since 2019: https://github.com/rust-analyzer/rust-analyzer/issues/2414
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.
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.
The only reason I'm okay with a
default.nix
in this repo is that I trust @FintanH will fix it when it breaks.
:innocent:
I suppose this works since #666?
Yes! Everything works fine now.
The integration tests in
librad
depend on thelibrad-test
crate which in turn depends on thelibrad
library. While cargo can deal with this circular dependecy rust-analyzer cannot. For all imports in oflibrad
inlibrad-test
I seeTo address this I’d suggest moving the integration tests from
librad/tests
tolibrad-test/tests
.