Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

client_changing_workspace_lib_retains_diagnostics fails frequently on slow drives #1466

Closed
ehuss opened this issue May 29, 2019 · 2 comments · Fixed by #1482
Closed

client_changing_workspace_lib_retains_diagnostics fails frequently on slow drives #1466

ehuss opened this issue May 29, 2019 · 2 comments · Fixed by #1482

Comments

@ehuss
Copy link
Contributor

ehuss commented May 29, 2019

The client_changing_workspace_lib_retains_diagnostics test has been failing occasionally on CI:

(possibly others, these are the ones that I can easily find)

I am able to reliably reproduce this error when using a magnetic spinning hard drive (around 80-90% failure rate) on both macos and windows.

After looking at the test, I'm fairly certain the reason is because the tests are sharing the cargo target directory. If one test (such as client_changing_workspace_lib_retains_diagnostics) builds something called "liblibrary", and another test (say client_test_complete_self_crate_name) creates a "liblibrary" at the same time, they will conflict with one another and stomp on each other's files.

I think the solution is that each test should use a dedicated target directory (that is how cargo's test suite works). I'm actually not sure how rls sets the target directory, or why the test suite is set up to reuse it, otherwise I would submit a PR. The following quick-and-dirty hack fixes the problem:

diff --git a/tests/support/client/mod.rs b/tests/support/client/mod.rs
index c563c98..0dcf1d0 100644
--- a/tests/support/client/mod.rs
+++ b/tests/support/client/mod.rs
@@ -58,6 +58,7 @@ impl Project {
     pub fn rls_cmd(&self) -> Command {
         let mut cmd = Command::new(rls_exe());
         cmd.current_dir(self.root());
+        cmd.env("CARGO_TARGET_DIR", self.root().join("target").to_string_lossy().into_owned());
         cmd.stderr(Stdio::inherit());
 
         cmd
mati865 pushed a commit to mati865/rls that referenced this issue Jun 7, 2019
…7, r=Xanewok

Bump serde from 1.0.85 to 1.0.87

Bumps [serde](https://github.com/serde-rs/serde) from 1.0.85 to 1.0.87.
<details>
<summary>Release notes</summary>

*Sourced from [serde's releases](https://github.com/serde-rs/serde/releases).*

> ## v1.0.87
> - Fix inclusion of tag in struct containing flattened fields ([rust-lang#1468](https://github-redirect.dependabot.com/serde-rs/serde/issues/1468), thanks [**jwillbold**](https://github.com/jwillbold))
>
> ## v1.0.86
> - Implement Serialize and Deserialize for core::ops::Bound\<T\> ([rust-lang#1466](https://github-redirect.dependabot.com/serde-rs/serde/issues/1466), thanks [**0nkery**](https://github.com/0nkery))
</details>
<details>
<summary>Commits</summary>

- [`134f268`](serde-rs/serde@134f268) Release 1.0.87
- [`c473633`](serde-rs/serde@c473633) Format with rustfmt 2018-12-10
- [`6a3a820`](serde-rs/serde@6a3a820) Merge pull request [rust-lang#1474](https://github-redirect.dependabot.com/serde-rs/serde/issues/1474) from jwillbold/master
- [`1d6ef76`](serde-rs/serde@1d6ef76) Fixed [rust-lang#1468](https://github-redirect.dependabot.com/serde-rs/serde/issues/1468), flattened struct fields made structs ignore their tag
- [`c8e3959`](serde-rs/serde@c8e3959) Release 1.0.86
- [`796f412`](serde-rs/serde@796f412) Document that Bound<T> impls exist
- [`fa854a2`](serde-rs/serde@fa854a2) Format with rustfmt 2018-12-10
- [`3a097ff`](serde-rs/serde@3a097ff) Deserialize Bound::Unbounded as unit variant
- [`8463bfc`](serde-rs/serde@8463bfc) Remove as yet unrequested range impls
- [`7a72b4c`](serde-rs/serde@7a72b4c) Merge pull request [rust-lang#1466](https://github-redirect.dependabot.com/serde-rs/serde/issues/1466) from 0nkery/master
- Additional commits viewable in [compare view](serde-rs/serde@v1.0.85...v1.0.87)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=serde&package-manager=cargo&previous-version=1.0.85&new-version=1.0.87)](https://dependabot.com/compatibility-score.html?dependency-name=serde&package-manager=cargo&previous-version=1.0.85&new-version=1.0.87)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the `.dependabot/config.yml` file in this repo:
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
mati865 pushed a commit to mati865/rls that referenced this issue Jun 7, 2019
…e-1.0.87, r=Xanewok

Bump serde_derive from 1.0.85 to 1.0.87

Bumps [serde_derive](https://github.com/serde-rs/serde) from 1.0.85 to 1.0.87.
<details>
<summary>Release notes</summary>

*Sourced from [serde_derive's releases](https://github.com/serde-rs/serde/releases).*

> ## v1.0.87
> - Fix inclusion of tag in struct containing flattened fields ([rust-lang#1468](https://github-redirect.dependabot.com/serde-rs/serde/issues/1468), thanks [**jwillbold**](https://github.com/jwillbold))
>
> ## v1.0.86
> - Implement Serialize and Deserialize for core::ops::Bound\<T\> ([rust-lang#1466](https://github-redirect.dependabot.com/serde-rs/serde/issues/1466), thanks [**0nkery**](https://github.com/0nkery))
</details>
<details>
<summary>Commits</summary>

- [`134f268`](serde-rs/serde@134f268) Release 1.0.87
- [`c473633`](serde-rs/serde@c473633) Format with rustfmt 2018-12-10
- [`6a3a820`](serde-rs/serde@6a3a820) Merge pull request [rust-lang#1474](https://github-redirect.dependabot.com/serde-rs/serde/issues/1474) from jwillbold/master
- [`1d6ef76`](serde-rs/serde@1d6ef76) Fixed [rust-lang#1468](https://github-redirect.dependabot.com/serde-rs/serde/issues/1468), flattened struct fields made structs ignore their tag
- [`c8e3959`](serde-rs/serde@c8e3959) Release 1.0.86
- [`796f412`](serde-rs/serde@796f412) Document that Bound<T> impls exist
- [`fa854a2`](serde-rs/serde@fa854a2) Format with rustfmt 2018-12-10
- [`3a097ff`](serde-rs/serde@3a097ff) Deserialize Bound::Unbounded as unit variant
- [`8463bfc`](serde-rs/serde@8463bfc) Remove as yet unrequested range impls
- [`7a72b4c`](serde-rs/serde@7a72b4c) Merge pull request [rust-lang#1466](https://github-redirect.dependabot.com/serde-rs/serde/issues/1466) from 0nkery/master
- Additional commits viewable in [compare view](serde-rs/serde@v1.0.85...v1.0.87)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=serde_derive&package-manager=cargo&previous-version=1.0.85&new-version=1.0.87)](https://dependabot.com/compatibility-score.html?dependency-name=serde_derive&package-manager=cargo&previous-version=1.0.85&new-version=1.0.87)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the `.dependabot/config.yml` file in this repo:
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
@Xanewok
Copy link
Member

Xanewok commented Jun 8, 2019

Hm, interesting, thanks for looking into this!

Right now we have a project setup harness copied some time ago from Cargo, which is now at https://github.com/rust-lang/rls/blob/de8a00e3f762cfebbf949f3ca665a585c61fe65b/tests/support/paths.rs.
As I understand it, this does separate the target directory - all the tests have their root dir set to target/rlsit/t${NUM} (and thus their target dir is at target/rlsit/t${NUM}/${PROJECT_NAME}/target).

Am I missing something?

@ehuss
Copy link
Contributor Author

ehuss commented Jun 8, 2019

Oh! I was slightly mistaken. It's that the test suite doesn't clear the CARGO_TARGET_DIR. When the rls tests are run in the rust repo, rustbuild sets CARGO_TARGET_DIR to build/$HOST/stage2-tools. Each test was reusing the same target directory. I have posted #1482 for a fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants