Skip to content

feat: add proc-macro rebuild on save option #16011

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

Merged

Conversation

ClSlaid
Copy link
Contributor

@ClSlaid ClSlaid commented Dec 4, 2023

Related: #15033

I need some advice on how to test it.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2023
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding testing, we don't really have the infra for that yet. So manual testing that it works suffices. https://github.com/rust-lang/rust-analyzer/blob/0840038f02daec6ba3238f05d8caa037d28701a0/docs/dev/debugging.md

You will also need to run the rust-analyzer crate tests to regenerate the config,json

Comment on lines 354 to 357
if self.proc_macro_changed && self.config.script_rebuild_on_save() {
self.fetch_build_data_queue
.request_op(format!("proc-macro or build script source changed"), ())
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so there is a problem with this for us here. Currently we set the proc_macro_changed flag whenever a change comes in for one of the sources, but those changes are not necessarily commited yet (saved), so with the current setup here we would ask for server rebuilds on every keystroke! We should instead move the op request to the DidSaveTextDocument handler (and then also reset the proc_macro_changed flag

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2023
@Veykril Veykril marked this pull request as ready for review January 2, 2024 09:27
@Veykril
Copy link
Member

Veykril commented Jan 2, 2024

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jan 2, 2024

📌 Commit c99089c has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 2, 2024

⌛ Testing commit c99089c with merge f1de7d7...

@bors
Copy link
Contributor

bors commented Jan 2, 2024

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

@bors bors merged commit f1de7d7 into rust-lang:master Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants