feat: Drop support for non-syroot proc macro ABIs #14432

Veykril · 2023-03-29T08:57:44Z

This makes some bigger changes to how we handle the proc-macro-srv things, for one it is now an empty crate if built without the sysroot-abi feature, this simplifies some things dropping the need to put the feature cfg in various places. The cli wrapper now actually depends on the server, instead of being part of the server that is just exported, that way we can have a true dummy server that just errors on each request if no sysroot support was specified.

lnicola · 2023-03-29T09:01:45Z

Maybe let's merge this next Monday so we can put up a warning before it happens.

Veykril · 2023-03-29T09:02:48Z

Certainly, I am not proposing to merge it this week, I am also fine with waiting a few more weeks. I just wanted to get this out of the way and see what changes (and if we can make the proc-macro tests work on our repo somehow)

crates/proc-macro-srv/src/proc_macros.rs

Veykril · 2023-03-29T10:17:41Z

Alright, proc-macro-srv tests should work on our CI now I think (with the help of my beloved RUSTC_BOOTSTRAP)

Veykril · 2023-03-30T14:23:05Z

Something breaks on mac here but I don't know what or why and since I don't have access to a mac myself I am not able to debug this ...

Veykril · 2023-04-04T17:14:20Z

Well, apparently it was buildscripts being a problem for the platform (I imagine its an architecture mismatch, iirc mac had some weird things with that). CI now works though 🎉

Veykril · 2023-04-04T17:38:56Z

So I guess now the question remains when we wanna publish this

Veykril · 2023-04-06T07:57:54Z

So, I wonder if we need to announce this at all, we already didn't do that the last time which dropped a lot of versions, while this one effectively drops one minor rust version only

lnicola

Right, the sysroot server was added in 1.64. I think we can merge this.

lnicola · 2023-04-06T08:04:48Z

@bors r+

bors · 2023-04-06T08:04:50Z

📌 Commit 33e649d has been approved by lnicola

It is now in the queue for this repository.

bors · 2023-04-06T08:04:58Z

⌛ Testing commit 33e649d with merge e3e324d...

bors · 2023-04-06T08:25:12Z

☀️ Test successful - checks-actions
Approved by: lnicola
Pushing e3e324d to master...

Noratrieb · 2023-04-10T08:09:45Z

.github/workflows/ci.yaml

@@ -24,6 +24,9 @@ jobs:
    runs-on: ${{ matrix.os }}
    env:
      CC: deny_c
+      # we want to build r-a on stable to check that it keeps building on stable,
+      # but we also want to test our proc-macro-srv which depends on nightly features
+      RUSTC_BOOTSTRAP: 1


I think you can set RUSTC_BOOTSTRAP to a set of crates like RUSTC_BOOTSTRAP=proc-macro-srv?

jonhoo · 2023-04-25T18:01:37Z

Should this new feature also be enabled in

rust-analyzer/xtask/src/install.rs

Line 139 in 943d2a8

    
           let cmd = cmd!(sh, "cargo install --path crates/rust-analyzer --locked --force --features force-always-assert {features...}");

so it gets built when using xtask install too?

Veykril · 2023-04-25T18:08:12Z

That would force people to use a nightly toolchain for install to work since the proc-macro server requires nightly features, so I don't think we want that to be appended there by default

flodiebold · 2023-04-25T19:44:51Z

It's also not necessary, right? It only needs to be enabled when building the proc macro server, which you only need to do when building rustc yourself, not if you just want to build rust-analyzer.

jonhoo · 2023-04-25T19:53:02Z

Ah, I missed that sysroot-abi required nightly to build! That helps explain the discrepancy.

In terms of whether it's necessary, you'll know that better than I do. But internally at $work where we weren't building rust-analyzer with sysroot-abi (since it was recently added), I now have folks running into this error:

rust-analyzer/crates/proc-macro-srv-cli/src/lib.rs

Lines 32 to 37 in 943d2a8

    
           msg::Request::ListMacros { .. } => { 
        
               msg::Response::ListMacros(Err("server is built without sysroot support".to_owned())) 
        
           } 
        
           msg::Request::ExpandMacro(..) => msg::Response::ExpandMacro(Err(msg::PanicMessage( 
        
               "server is built without sysroot support".to_owned(), 
        
           ))),

Which suggests that for at least some of the things they're wanting to do this comes back to bite them.

Veykril · 2023-04-25T19:55:49Z

Which rust version are you using at $work? If it is < 1.64 then yes, you'll have to build r-a with this feature (and pray that the proc-macro interface hasn't changed since your version, that is the proc-macro-srv might not build on < 1.64 due to API breakage given its unstable)

flodiebold · 2023-04-25T19:56:16Z

That code lives inside the proc macro server, and unless I'm misunderstanding how this works now (which is possible), you should be running the proc macro server bundled with your rustc installation. So you should only run into this error if you're using a sysroot that doesn't have the bundled proc macro server, which should only happen if your rustc is too old or you built it yourself, I think. (I guess we fall back to using rust-analyzer as the proc macro server, which is how you then get the error message?)

Veykril · 2023-04-25T19:57:40Z

(I guess we fall back to using rust-analyzer as the proc macro server, which is how you then get the error message?)

Yes, if we can't find a sysroot server we fall back to using the r-a server binary which I believe is built without the feature when publishing? (which makes sense fwiw, but I am actually not 100% sure that's the case)

flodiebold · 2023-04-25T19:59:37Z

I wonder if that fallback still makes sense then? I guess it can be useful if you built RA with the feature, but it's also just likely to break then if you're not using the exact right rustc version. Maybe we should just give an error message on the server side instead?

Veykril · 2023-04-25T20:20:04Z

We should probably get rid of the fallback yes, now that we only support the sysroot the fallback makes little sense.

andrewbanchich · 2023-04-25T20:42:34Z

Which rust version are you using at $work? If it is < 1.64 then yes, you'll have to build r-a with this feature (and pray that the proc-macro interface hasn't changed since your version, that is the proc-macro-srv might not build on < 1.64 due to API breakage given its unstable)

@Veykril 1.69.0

Veykril · 2023-04-25T20:46:49Z

Huh, are you setting the proc-macro server path specifically instead of letting it pick the sysroot one?

andrewbanchich · 2023-04-25T20:53:47Z

Nope, not doing anything fancy. Unless Emacs' lsp-mode is doing that under the hood for some reason.

jonhoo · 2023-04-25T21:09:58Z

We do build Rust from source though, so if there's a particular way in which we have to configure the Rust build to get it to build the proc macro server, then we may not be doing that? Does this assume that a particular rustup component is installed for example?

andrewbanchich · 2023-04-25T21:12:01Z

I uninstalled our internal build, cloned the repo, and installed using cargo xtask install --server and I'm still getting this error.

Veykril · 2023-04-25T21:17:24Z

We do build Rust from source though,

Does this mean you are using a custom toolchain? That might be the problem, I am not sure if that will build the proc-macro server by default.

Also seeing a server log would be nice, it might have some extra info on what server is being used.

(nevertheless, this change here should not have impacted you. From what you've described, you should've had different problems alltogether prior to this)

jonhoo · 2023-04-25T21:22:14Z

Yeah, there's a custom toolchain in use here. In more ways than one — it's both a custom rustup toolchain (a "path" toolchain in rust-toolchain.toml) and a custom build of Rust underlying that. Looking a little deeper here, the proc macro server lives under $sysroot/libexec, so I suspect our custom toolchain just doesn't distribute that subdirectory correctly. I'll do some more digging!

jonhoo · 2023-04-25T23:40:45Z

Seems that with rust-lang/rust@11e002a (which landed in 1.69), the proc macro server is no longer always built, and needs to be explicitly listed in the tools list for any custom from source builds of Rust 👍

Drop support for non-syroot proc macro ABIs

7498ec7

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2023

Run proc-macro-srv tests on rust-lang/rust-analyzer

afe52d2

Veykril commented Mar 29, 2023

View reviewed changes

crates/proc-macro-srv/src/proc_macros.rs Outdated Show resolved Hide resolved

Veykril added 2 commits March 29, 2023 11:32

Fix visibilities

c4582f6

Thread global symbol interner through as if it was a local

0522503

Fix proc-macro server spawning behavior when the server is r-a itself

af0d548

Veykril force-pushed the proc-macro-srv branch from 3d6087f to af0d548 Compare March 30, 2023 16:59

Veykril added 2 commits April 4, 2023 19:02

Adjust default config for slow-tests

1d085cf

Error out slow-tests when server loading errors

6644fe3

Veykril added 2 commits April 4, 2023 19:24

Show proc-macro spawn errors as status notification warnings

25635ad

Also panic on status warnings

235c909

Veykril marked this pull request as ready for review April 4, 2023 17:26

Allow warnings in out_dirs_check

33e649d

Veykril force-pushed the proc-macro-srv branch from b022549 to 33e649d Compare April 5, 2023 08:30

lnicola approved these changes Apr 6, 2023

View reviewed changes

bors merged commit e3e324d into rust-lang:master Apr 6, 2023

Veykril deleted the proc-macro-srv branch April 6, 2023 08:37

lnicola changed the title ~~Drop support for non-syroot proc macro ABIs~~ feat: Drop support for non-syroot proc macro ABIs Apr 6, 2023

Noratrieb reviewed Apr 10, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Drop support for non-syroot proc macro ABIs #14432

feat: Drop support for non-syroot proc macro ABIs #14432

Veykril commented Mar 29, 2023 •

edited

Loading

lnicola commented Mar 29, 2023

Veykril commented Mar 29, 2023

Veykril commented Mar 29, 2023

Veykril commented Mar 30, 2023

Veykril commented Apr 4, 2023

Veykril commented Apr 4, 2023

Veykril commented Apr 6, 2023

lnicola left a comment

lnicola commented Apr 6, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

Noratrieb Apr 10, 2023

jonhoo commented Apr 25, 2023

Veykril commented Apr 25, 2023

flodiebold commented Apr 25, 2023

jonhoo commented Apr 25, 2023

Veykril commented Apr 25, 2023

flodiebold commented Apr 25, 2023

Veykril commented Apr 25, 2023

flodiebold commented Apr 25, 2023

Veykril commented Apr 25, 2023

andrewbanchich commented Apr 25, 2023

Veykril commented Apr 25, 2023

andrewbanchich commented Apr 25, 2023

jonhoo commented Apr 25, 2023

andrewbanchich commented Apr 25, 2023 •

edited

Loading

Veykril commented Apr 25, 2023 •

edited

Loading

jonhoo commented Apr 25, 2023

jonhoo commented Apr 25, 2023

feat: Drop support for non-syroot proc macro ABIs #14432

feat: Drop support for non-syroot proc macro ABIs #14432

Conversation

Veykril commented Mar 29, 2023 • edited Loading

lnicola commented Mar 29, 2023

Veykril commented Mar 29, 2023

Veykril commented Mar 29, 2023

Veykril commented Mar 30, 2023

Veykril commented Apr 4, 2023

Veykril commented Apr 4, 2023

Veykril commented Apr 6, 2023

lnicola left a comment

Choose a reason for hiding this comment

lnicola commented Apr 6, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

bors commented Apr 6, 2023

Noratrieb Apr 10, 2023

Choose a reason for hiding this comment

jonhoo commented Apr 25, 2023

Veykril commented Apr 25, 2023

flodiebold commented Apr 25, 2023

jonhoo commented Apr 25, 2023

Veykril commented Apr 25, 2023

flodiebold commented Apr 25, 2023

Veykril commented Apr 25, 2023

flodiebold commented Apr 25, 2023

Veykril commented Apr 25, 2023

andrewbanchich commented Apr 25, 2023

Veykril commented Apr 25, 2023

andrewbanchich commented Apr 25, 2023

jonhoo commented Apr 25, 2023

andrewbanchich commented Apr 25, 2023 • edited Loading

Veykril commented Apr 25, 2023 • edited Loading

jonhoo commented Apr 25, 2023

jonhoo commented Apr 25, 2023

Veykril commented Mar 29, 2023 •

edited

Loading

andrewbanchich commented Apr 25, 2023 •

edited

Loading

Veykril commented Apr 25, 2023 •

edited

Loading