Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a way to disable dll copying for users of proc_macro_srv library #11365

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

vlad20012
Copy link
Member

@vlad20012 vlad20012 commented Jan 28, 2022

We use ra_ap_proc_macro_srv library in IntelliJ Rust in order to expand proc macros. We need a way to disable DLL copying to a temp dir on Windows behavior because it causes issues like intellij-rust/intellij-rust#7709. Unlike RA, file locking is not an issue for IntelliJ Rust because we copy DLLs to a temp dir before calling the expander.

@lnicola
Copy link
Member

lnicola commented Jan 28, 2022

My slight preference would be to check an environment variable instead of using a Cargo feature, but it's not that important.

@vlad20012
Copy link
Member Author

vlad20012 commented Jan 28, 2022

It's ok to use an env. if you prefer an env, I can rewrite the implementation

@lnicola
Copy link
Member

lnicola commented Jan 28, 2022

Unrelated, which ABIs would you like to support in IntelliJ Rust? We currently support Rust 1.47 and later, but the original plan was to only have one stable version.

@vlad20012
Copy link
Member Author

Unrelated, which ABIs would you like to support in IntelliJ Rust? We currently support Rust 1.47 and later, but the original plan was to only have one stable version.

I think we anyway can link multiple ra_ap_proc_macro_srv versions if we want to support more rust versions. But yes, we currently want to support as many versions as possible.

Btw, I think the term "ABI" is not correct in this case, because rustc always use "C" ABI for proc macros. This is actually API changes

@lnicola
Copy link
Member

lnicola commented Jan 28, 2022

bors r+

@bjorn3
Copy link
Member

bjorn3 commented Jan 28, 2022

Btw, I think the term "ABI" is not correct in this case, because rustc always use "C" ABI for proc macros. This is actually API changes

I think it is actually correct. The ABI is what actually happens at binary level after compilation. The API is what a programmer sees. The API would be the public interface of the proc_macro crate, while the ABI is how proc_macro how it communicates with rustc behind the scenes. The ABI changes between rustc versions, but the stable part of the API remains identical so if you recompile the proc-macro it will remain working for a newer rustc version.

@bors
Copy link
Contributor

bors bot commented Jan 28, 2022

@bors bors bot merged commit 4aadabc into rust-lang:master Jan 28, 2022
@vlad20012
Copy link
Member Author

@bjorn3
Ok, maybe not API, but a "protocol" or something like that. Like changes in JSON scheme that proc_macro_srv and proc_macro_api use.
ABI is a specific well-defined thing and proc macros use stable "C" ABI since rust-lang/rust#49219 (previously it was unstable Rust ABI)

@lnicola
Copy link
Member

lnicola commented Jan 28, 2022

Even in a C library, if you make changes to a public struct, for example, that's an ABI break and you're supposed to bump the soname.

Not that it matters too much here :-).

@vlad20012
Copy link
Member Author

Ok, it looks like I interpret the ABI term too narrowly - something like "calling conventions". If we say that API changes lead to ABI changes, then yes, it is ABI changes.

Ok, I will call this ABI versions/ABIs too :)

bors bot added a commit to intellij-rust/intellij-rust that referenced this pull request Jan 31, 2022
8489: Update `ra_ap_proc_macro_srv` to `0.0.94`, fix disk usage, fix pwd r=vlad20012 a=vlad20012

This update consists of rust-lang/rust-analyzer#11365, rust-lang/rust-analyzer#11353 and rust-lang/rust-analyzer#11356 PRs

Fixes #7709
Fixes #8238 in a proper way (#8271 was a hacky hotfix)

changelog: Don't fill up disk space by proc macro DLLs on Windows

Co-authored-by: vlad20012 <beskvlad@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants