Read version of rustc that compiled proc macro #6822

jsomedon · 2020-12-11T14:28:19Z

Signed-off-by: Jay Somedon [email protected]

This PR is to fix #6174.

I basically

added two methods, read_version and read_section(used by read_version)
two new crates snap and object to be used by those two methods

I just noticed that some part of code were auto-reformatted by rust-analyzer on file save. Does it matter?

crates/proc_macro_api/Cargo.toml

crates/proc_macro_api/src/lib.rs

bjorn3 · 2020-12-11T14:34:57Z

crates/proc_macro_api/src/lib.rs

+        snappy_decoder
+            .read_exact(&mut bytes_before_version)
+            .unwrap();
+        let length = bytes_before_version[12]; // what? can't use -1 indexing?


This should probably also check that the metadata version is 5.

Or rather, it should be checked before decompression.

You mean, the last byte 5 in 8 bytes sequence r u s t 0 0 0 5?

I think this is resolved by the condition check if header != EXPECTED_HEADER {...} that I added in later commit? @bjorn3

I think this is resolved by the condition check if header != EXPECTED_HEADER {...} that I added in later commit?

Yes, I think so .

crates/proc_macro_api/src/lib.rs

lnicola · 2020-12-11T14:35:57Z

There were some changes in this crate (dummy is now gone), so you should try to rebase in some way or another. And maybe try to avoid unwraps just in case. You can probably return io::Error like in #6817.

jsomedon · 2020-12-11T14:38:51Z

If I keep add commit to this branch on my laptop then push them to my fork on github, do those change get automatically updated here?

lnicola · 2020-12-11T14:39:24Z

If I keep add commit to this branch on my laptop then push them to my fork on github, do those change get automatically updated here?

Yes

jsomedon · 2020-12-11T14:40:21Z

If I keep add commit to this branch on my laptop then push them to my fork on github, do those change get automatically updated here?

Yes

noice!

lnicola · 2020-12-11T14:40:26Z

See #6393 (comment) if some rebasing tips, but it's fine if you don't feel like trying that.

jsomedon · 2020-12-11T14:42:30Z

See #6393 (comment) if some rebasing tips, but it's fine if you don't feel like trying that.

👀

jsomedon · 2020-12-11T14:55:26Z

And maybe try to avoid unwraps just in case. You can probably return io::Error like in #6817.

I see. Is it ok to use ?? EDIT: I think it's okay, I saw bunch of ? use case in that link. 😁

lnicola · 2020-12-11T14:58:18Z

It's ok, but you'll need to use a bunch of .map-err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?.

matklad · 2020-12-11T15:12:53Z

r? @jonas-schievink :)

lnicola · 2020-12-20T07:40:19Z

@jsomedon hey, do you need help with the rebase?

jsomedon · 2020-12-20T14:14:54Z

@jsomedon hey, do you need help with the rebase?

My bad for disappearing without words.. I caught the flu so I was mostly taking rest these days instead of wrestling with rust code. Probably shouldn't skip sleeping 😓 I haven't look much onto what I need for rebasing but I guess I should cherry pick some commits just like you did in the comment you linked?

lnicola · 2020-12-20T14:18:45Z

Yes, you can cherry-pick the first one.

No worries, just shout if you need help.

lnicola · 2020-12-21T18:41:14Z

@bjorn3 do you think this is worth the complexity over reading the output of rustc -V?

bjorn3 · 2020-12-21T18:44:02Z

🤷

jonas-schievink · 2020-12-21T19:03:28Z

I think it is – running rustc might not work or might invoke the wrong compiler when using project.json

jsomedon · 2020-12-23T13:43:31Z

I just pushed commits to resolve issues mentioned in code review. Will work on the rebasing task.

crates/proc_macro_api/src/lib.rs

jonas-schievink · 2020-12-23T13:50:24Z

crates/proc_macro_api/src/lib.rs

+    // * [some more bytes that we don really care but still there] :-)
+    // Check this issue for more about the bytes layout:
+    // https://github.com/rust-analyzer/rust-analyzer/issues/6174
+    fn read_version(&self, dylib_path: &Path) -> io::Result<String> {


Is this actually getting called somewhere?

Not yet, I think. We need to call it before loading the library, around ProcMacroLibraryLibloading::open or above.

But we still need to figure out what version to expect.

No, but there is this discussion.

Well, it would still be nice to wire it up. I just didn't want to scare you off right from the start 😬

Well, it would still be nice to wire it up. I just didn't want to scare you off right from the start grimacing

I see, sure thing. 😄

That won't quite work for beta and stable, as those don't show the date at which the branch forked from master (and thus abi compatibility) but the date at which they were built.

Oh, right. We need three version strings, then?

I hate this.

(confused and scratching my head)

So how do I get the version that I am comparing with for stable and beta? And is there universal way of doing this for all three including nightly?

We have some discussion in zulip that only stable is fine for now.

lnicola · 2020-12-23T14:30:24Z

So I think the last ABI change was rust-lang/rust#74653, which means the earliest rustc we support is... the nightly from 2020-07-28?

lnicola · 2020-12-23T14:34:07Z

rustc 1.47.0-nightly (6c8927b0c 2020-07-26) doesn't work
rustc 1.47.0-nightly (76e83339b 2020-07-27) works

jsomedon · 2020-12-23T15:20:22Z

Just updated my local repo and github fork, then rebased my fix branch(jsomedon:check_version), then git push --forced. The git push includes copies of 5 previous commits and 1 new commit that removes one extra entry for object crate in Cargo.lock(probably added twice during rebase)

crates/proc_macro_api/src/lib.rs

edwin0cheng

@jsomedon

Sorry for the delay. We want to push forward this PR for #7328 now.

I think current form is okay now (we could refactoring later on), and the miss piece of this PR is actually call read_version from ProcMacroLibraryLibloading::open and logging warning if the version is invalid.

If you are busy, I could co-author this PR if you want.

edwin0cheng · 2021-03-01T17:21:36Z

crates/proc_macro_api/src/lib.rs

+    // * [some more bytes that we don really care but still there] :-)
+    // Check this issue for more about the bytes layout:
+    // https://github.com/rust-analyzer/rust-analyzer/issues/6174
+    fn read_version(&self, dylib_path: &Path) -> io::Result<String> {


We have some discussion in zulip that only stable is fine for now.

edwin0cheng · 2021-03-01T17:22:17Z

crates/proc_macro_api/src/lib.rs

+        snappy_decoder
+            .read_exact(&mut bytes_before_version)
+            .unwrap();
+        let length = bytes_before_version[12]; // what? can't use -1 indexing?


I think this is resolved by the condition check if header != EXPECTED_HEADER {...} that I added in later commit?

Yes, I think so .

jsomedon · 2021-03-02T04:13:14Z

If you are busy, I could co-author this PR if you want.

Yeah, sure. 😅
~~I guess I should do something so that you have write access to this PR, or is it done by github automatically?~~
nvm I think I saw that "allow edit for maintainer" checkbox automatically checked on on this page

With Jay Somedon <[email protected]>

Signed-off-by: Jay Somedon <[email protected]>

* check metadata version * use memmap * use Result instead of unwrap with Jay Somedon <[email protected]>

Co-authored-by: Jonas Schievink <[email protected]>

Co-authored-by: Laurențiu Nicola <[email protected]>

edwin0cheng · 2021-03-04T02:54:34Z

@jsomedon basically I just rebased and add the missing part (print out warning if the version is too old) of your PR.

r? @lnicola

bjorn3 · 2021-03-04T06:44:32Z

There is a typo in the commit message: *built

matklad · 2021-03-09T19:06:23Z

LGTM

@edwin0cheng is this waiting for anything else?

crates/proc_macro_api/src/version.rs

edwin0cheng · 2021-03-09T20:56:49Z

bors r+

edwin0cheng · 2021-03-09T20:58:48Z

@jsomedon Thanks for the PR !

bors · 2021-03-09T21:07:22Z

Build succeeded:

lnicola · 2021-04-08T10:17:53Z

Did the metadata format change recently?

proc-macro target/debug/deps/libderivative-0649283d66d689a0.so failed to find the given version. Reason: snappy: corrupt input (expected stream header but got unexpected chunk type byte 228)

lnicola · 2021-04-08T10:21:39Z

Never mind, I ended up on an old toolchain. I guess that's another thing to check for :-(.

8716: Replace `memmap` with `memmap2` in `proc_macro_api` r=edwin0cheng a=memoryruins #7522 did the same for `proc_macro_srv` before this usage of `memmap` was introduced to `proc_macro_api` in #6822 . Something like [`cargo-deny`](https://github.com/EmbarkStudios/cargo-deny-action) could help prevent specific crates (and versions, licenses, etc) from being introduced into the crate tree, but that's unrelated to this pull request. Co-authored-by: memoryruins <[email protected]>

bjorn3 reviewed Dec 11, 2020

View reviewed changes

crates/proc_macro_api/Cargo.toml Outdated Show resolved Hide resolved

lnicola reviewed Dec 11, 2020

View reviewed changes

crates/proc_macro_api/src/lib.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Dec 11, 2020

View reviewed changes

lnicola reviewed Dec 11, 2020

View reviewed changes

crates/proc_macro_api/src/lib.rs Outdated Show resolved Hide resolved

jonas-schievink reviewed Dec 23, 2020

View reviewed changes

lnicola reviewed Dec 24, 2020

View reviewed changes

crates/proc_macro_api/src/lib.rs Outdated Show resolved Hide resolved

edwin0cheng mentioned this pull request Jan 18, 2021

Enable proc macros by default #7328

Closed

edwin0cheng reviewed Mar 1, 2021

View reviewed changes

jsomedon added 2 commits March 4, 2021 09:05

Read version of rustc that compiled proc macro

6608ace

With Jay Somedon <[email protected]>

Configure object crate's feature

8fd7cd7

Signed-off-by: Jay Somedon <[email protected]>

jsomedon and others added 4 commits March 4, 2021 09:11

Fix multiple issues from code review

55d73bc

* check metadata version * use memmap * use Result instead of unwrap with Jay Somedon <[email protected]>

Update comment

c92db2a

Co-authored-by: Jonas Schievink <[email protected]>

Update condition check code style

a8f7326

Co-authored-by: Jonas Schievink <[email protected]>

Revise error message regarding metadata version

0669abd

Co-authored-by: Laurențiu Nicola <[email protected]>

Print warning if proc-macro built by old rustc

cc8c404

bjorn3 reviewed Mar 9, 2021

View reviewed changes

crates/proc_macro_api/src/version.rs Outdated Show resolved Hide resolved

use doc-comments

ad34e79

bors bot merged commit 654313d into rust-lang:master Mar 9, 2021

lnicola mentioned this pull request Apr 9, 2021

proc macro server crashes for certain macros #8148

Closed

memoryruins mentioned this pull request May 4, 2021

Replace memmap with memmap2 in proc_macro_api #8716

Merged

Read version of rustc that compiled proc macro #6822

Read version of rustc that compiled proc macro #6822

Uh oh!

Conversation

jsomedon commented Dec 11, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lnicola commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsomedon commented Dec 11, 2020

Uh oh!

lnicola commented Dec 11, 2020

Uh oh!

jsomedon commented Dec 11, 2020

Uh oh!

lnicola commented Dec 11, 2020

Uh oh!

jsomedon commented Dec 11, 2020

Uh oh!

jsomedon commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lnicola commented Dec 11, 2020

Uh oh!

matklad commented Dec 11, 2020

Uh oh!

lnicola commented Dec 20, 2020

Uh oh!

jsomedon commented Dec 20, 2020

Uh oh!

lnicola commented Dec 20, 2020

Uh oh!

lnicola commented Dec 21, 2020

Uh oh!

bjorn3 commented Dec 21, 2020

Uh oh!

jonas-schievink commented Dec 21, 2020

Uh oh!

jsomedon commented Dec 23, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lnicola commented Dec 23, 2020

Uh oh!

lnicola commented Dec 23, 2020

lnicola commented Dec 11, 2020 •

edited

Loading

jsomedon commented Dec 11, 2020 •

edited

Loading

jsomedon commented Mar 2, 2021 •

edited

Loading