fix: Fix proc macro ABI version checks #10799
Conversation
10799: fix: Fix proc macro ABI version checks r=lnicola a=lnicola If I'm reading this right, we used to pick `Abi1_55` for `1.54` and `Abi_1_58` for `1.57`. CC `@alexjg` (just in case I'm misinterpreting the code), CC #10772. bors r+ Co-authored-by: Laurențiu Nicola <[email protected]>
|
The old code is correct too. It writes |
|
bors r- |
|
Canceled. |
|
Doesn't it pick 1.55 for 54? |
| let inner = unsafe { Abi_1_47::from_lib(lib, symbol_name) }?; | ||
| Ok(Abi::Abi1_47(inner)) | ||
| } else if info.version.1 < 56 { | ||
| let inner = unsafe { Abi_1_55::from_lib(lib, symbol_name) }?; |
There was a problem hiding this comment.
Say the minor version is 55, then < 54 is false but < 56 is true, so it ends up at this line, using the 1.55 abi.
There was a problem hiding this comment.
No, let's say the minor is 54.
There was a problem hiding this comment.
Right, then it still ends up using 1.55, so the old code is wrong. In any case your code is much clearer.
|
I'm still slightly worried about what happens between version bumps (say around the beta versions), but I hope it's not worse than before. This fixes a crash with beta on my machine. bors r+ |
|
Maybe we're supporting too many ABIs? 😬 |
|
I think there are a few 1.58 nightlies(the early ones) that use the non 1.58 abi which causes us to crash. At least that was what I experienced before updating my 1.58 install I believe. |
|
Yeah, probably, but we shouldn't go out of our way to support older nightlies in the current (or in a previous) release cycle. I think someone asked for this (in the current release cycle), but it felt like it was some sort of an XY problem. EDIT: that was @Dessix in #9550 (comment).
To put it clearly, I'm against it because adding support for a new ABI is something that lately only @alexjg has done. Most code is lifted from |
|
I can write a few extra docs on the process for adding a new ABI. It's fairly mechanical. |
11187: Rename and use the 1.55 ABI for 1.54 r=lnicola a=lnicola It seems that what we used to call the 1.55 ABI was actually introduced in 1.54. CC #10799 Thanks to `@danielframpton` for finding it. Co-authored-by: Laurențiu Nicola <[email protected]>
If I'm reading this right, we used to pick
Abi1_55for1.54andAbi_1_58for1.57.CC @alexjg (just in case I'm misinterpreting the code), CC #10772.
bors r+