Skip to content

Process cleanup - #3877

Merged
djc merged 5 commits into
masterfrom
process-cleanup
Jun 16, 2024
Merged

Process cleanup#3877
djc merged 5 commits into
masterfrom
process-cleanup

Conversation

@djc

@djcdjc commented Jun 16, 2024

Copy link
Copy Markdown
Contributor

This is from last night. I think the custom test macros are unnecessary now, please review!

@djc
djc requested a review from rami3lJune 16, 2024 06:16
@djc
djcforce-pushed the process-cleanup branch from 73f9225 to 1dee499CompareJune 16, 2024 06:17
Comment threadsrc/bin/rustup-init.rs Outdated

@rami3lrami3l left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wow that's soooo impressive, friend!

Rustup is finally an "ordinary" tokio app now thanks to your work! Hooray 🎉

@djc

djc commented Jun 16, 2024

Copy link
Copy Markdown
ContributorAuthor

Rustup is finally an "ordinary" tokio app now thanks to your work! Hooray 🎉

Our work, couldn't have done it without you!

@djc
djcforce-pushed the process-cleanup branch from 0796d14 to 4ef3559CompareJune 16, 2024 08:48
@djc
djc enabled auto-merge June 16, 2024 08:49
@rami3lrami3l added this to the 1.28.0 milestone Jun 16, 2024
@djc
djc added this pull request to the merge queue Jun 16, 2024
@rami3l

rami3l commented Jun 16, 2024

Copy link
Copy Markdown
Member

@djc Let's don't forget there's one remaining concern from #3847 (comment): there are places where the original code was using #[::tokio::test(flavor = "multi_thread", worker_threads = 1)] (you just deleted these lines in this PR), while worker_threads = 2 was used in with_runtime() instead:

letmut builder = Builder::new_multi_thread();
builder
.enable_all()
.worker_threads(2)
.max_blocking_threads(2);
let process_res = currentprocess::with_runtime(
tp.clone().into(),
builder,
rustup_mode::main(tp.cwd.clone()),
);

I'm not quite sure about the exact intention of those configurations, but it's better to be watchful I think.

Merged via the queue into master with commit a10ad73Jun 16, 2024
@djc
djc deleted the process-cleanup branch June 16, 2024 09:32
@rami3lrami3l mentioned this pull request Oct 6, 2024
3 tasks
Sign up for freeto 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.

2 participants

@djc@rami3l