Skip to content

Conversation

poliorcetics
Copy link
Contributor

@poliorcetics poliorcetics commented Sep 24, 2022

This PR adds support for the ability to pass multiple --target flags when using
cargo 1.64.0+.

Questions

I needed to change the type of two configurations options, but I did not plurialize the names to
avoid too much churn, should I ?

Zulip thread

https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/Issue.2013282.20.28supporting.20multiple.20targets.20with.201.2E64.2B.29

Example

To see it working, on a macOS machine:

$ cd /tmp
$ cargo new cargo-multiple-targets-support-ra-test
$ cd !$
$ mkdir .cargo
$ echo '
[build]
target = [
    "aarch64-apple-darwin",
    "x86_64-apple-darwin",
]
' > .cargo/config.toml
$ echo '
fn main() {
    #[cfg(all(target_arch = "aarch64", target_os = "macos"))]
    {
        let a = std::fs::read_to_string("/tmp/test-read");
    }

    #[cfg(all(target_arch = "x86_64", target_os = "macos"))]
    {
        let a = std::fs::read_to_string("/tmp/test-read");
    }

    #[cfg(all(target_arch = "x86_64", target_os = "windows"))]
    {
        let a = std::fs::read_to_string("/tmp/test-read");
    }
}
' > src/main.rs
# launch your favorite editor with the version of RA from this PR
#
# You should see warnings under the first two `let a = ...` but not the third

Screen

Two panes of a terminal emulator, on the left pane is the main.rs file described above, with warnings for the first two let a = declaration, on the right pane is a display of the .cargo/config.toml, an ls of the current files in the directory and a call to cargo build to show the same warnings as in the editor on the left pane

Helps with #13282

@poliorcetics poliorcetics force-pushed the multiple-targets branch 2 times, most recently from 65e8b97 to abb3f0e Compare September 25, 2022 09:30
@poliorcetics
Copy link
Contributor Author

I didn't make the change for the rust-project.json config because rustc does not accept multiple --target flags, contrarily to cargo

@flodiebold
Copy link
Member

I don't think we can support this for cargo.target, at most for checkOnSave.

@poliorcetics
Copy link
Contributor Author

I don't think we can support this for cargo.target, at most for checkOnSave.

cargo on 1.64.0+ accepts multiple targets, it's even supported in the .cargo/config.toml, why would cargo.target not support it ?

@flodiebold
Copy link
Member

Because cargo.target controls our analysis, and that doesn't support multiple targets (without a lot more code).

@poliorcetics
Copy link
Contributor Author

Because cargo.target controls our analysis, and that doesn't support multiple targets (without a lot more code).

Oh, I'll try to do it, either in this PR if it's not merged by then, or in another. For now, I removed it from the commit and changed back to an Option.

@poliorcetics
Copy link
Contributor Author

Progress report

I made some progress on this today 🎉.

I have completions working for several targets at once:

std::arch::x86_64::__m12 correctly proposes the completion for __m128

But (because of course it's not perfectly working), completions for conflicting names fail, I don't which is chosen first (I suspect the first configured target):

sys_func(usize, usize) is selected instead of the single-argument variant, which is the one to use on x86_64-apple-darwin

@poliorcetics poliorcetics force-pushed the multiple-targets branch 2 times, most recently from 16c50a4 to 54efd42 Compare October 9, 2022 20:26
@poliorcetics
Copy link
Contributor Author

Go to definition and errors will point to the wrong item with the implementation I used, I removed that part and kept only the multiple targets when running cargo, not inside R-A

@bors
Copy link
Contributor

bors commented Oct 19, 2022

☔ The latest upstream changes (presumably #13128) made this pull request unmergeable. Please resolve the merge conflicts.

@Veykril Veykril added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2022
@poliorcetics poliorcetics force-pushed the multiple-targets branch 2 times, most recently from 39aa7b0 to 12f5d80 Compare November 9, 2022 11:28
@Veykril Veykril changed the title Support multiple targets (in conjunction with cargo 1.64.0+) Support multiple targets for checkOnSave (in conjunction with cargo 1.64.0+) Nov 19, 2022
@Veykril
Copy link
Member

Veykril commented Nov 19, 2022

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Nov 19, 2022

📌 Commit 0d4737a has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 19, 2022

⌛ Testing commit 0d4737a with merge 38fa47f...

@bors
Copy link
Contributor

bors commented Nov 19, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 38fa47f to master...

@bors bors merged commit 38fa47f into rust-lang:master Nov 19, 2022
@poliorcetics poliorcetics deleted the multiple-targets branch November 19, 2022 13:36
iredelmeier added a commit to iredelmeier/rust-analyzer that referenced this pull request Nov 21, 2022
This fixes a regression introduced by rust-lang#13290, in which failing to set
`checkOnSave/target` (or `checkOnSave/targets`) would lead to an invalid
config.
iredelmeier added a commit to iredelmeier/rust-analyzer that referenced this pull request Nov 21, 2022
This fixes a regression introduced by rust-lang#13290, in which failing to set
`checkOnSave/target` (or `checkOnSave/targets`) would lead to an invalid
config.
bors added a commit that referenced this pull request Nov 24, 2022
…as-schievink

Fix: Handle empty `checkOnSave/target` values

This fixes a regression introduced by #13290, in which failing to set `checkOnSave/target` (or `checkOnSave/targets`) would lead to an invalid config.

[Fixes #13660]
arcnmx added a commit to arcnmx/rust that referenced this pull request Dec 17, 2022
The prior update included checkOnSave multiple targets:
rust-lang/rust-analyzer#13290
but missed the fix for the regression it caused:
rust-lang/rust-analyzer#13661

Merge commit '6d61be8e65ac0fd45eaf178e1f7a1ec6b582de1f'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants