Pass Process around explicitly

[djc]

OTOH I do have a feeling that the overall Rust testing ecosystem as it stands is hostile to global states/injections, so when working on #3803, I'm constantly thinking maybe we should get rid of that part altogether. Maybe that's too radical though?
#3868

I think that's kind of what this is? Doesn't seem too radical to me.

I haven't quite finished the tests and the I'm not sure yet what to do about the logging macros, but this seems viable (and should enable ripping out a bunch of stuff like with() and with_runtime() etc).

[djc]

Here the only place that still calls process() is in cli::log. Presumably we'll want to replace those soon with tracing macros anyway, so not a big deal? I think if we stack your changes from #3868 on top of this things will be much better.

[rami3l]

@djc Thanks for the work! Looks like 84e01b5 is a rather mechanical change (replacing process() with process)? If that is the case, I think we can first merge the 2 commits before it.

Having noticed the fact that effectively PROCESS is only altered in with() and with_runtime(), both of which are used only in tests (except one single mention of with_runtime() in rustup-init.rs), my overall plan is that:

1. refactor(test): execute all #[rustup_macros::unit_test]s within a tokio context #3868 temporarily removes in-process tests, now subscribers can be totally local.
2. chore(dist/features): ship tracing and friends by default #3803 sets a global subscriber based on the current static PROCESS in normal business logic, and for tests it sets a local subscriber using let _guard = tracing::subscriber::set_default(subscriber);, where subscriber is a function of the current PROCESS. Clearly, the only injection points of the local subscriber would be inside the with*() functions (a new PROCESS will naturally require a new stderr-oriented subscriber). The log module now relies on the implicit subscriber in the current scope (global or local).
3. With this PR, PROCESS can be safely removed, since the info!() calls and friends only implicitly rely on the subscriber. The subscriber's dependency on the new process in this case can be specified elsewhere (globally, or locally using with()). Now with()'s only job is to install the hook and then change the current subscriber when the closure runs. since there's no PROCESS anymore and thus no more calls to .on_thread_start() and stuff to alter it, with_runtime() is no longer required. ¹ An async version of with() might still be desired though, but it won't have to depend on tokio runtime builder.
4. test(clitools): revive run_inprocess() #3891 revives in-process tests using just with() or its async counterpart.

What do you think?

Footnotes

1. I'm not sure about the parameters to be passed to the topmost tokio runtime builder yet. IIRC in with_runtime() 2 workers are used, in #[test] there's only one. ↩

[djc]

Sounds like a good direction! I'm not completely clear on which order you want to merge things?

If that is the case, I think we can first merge the 2 commits before it.

I've submitted them separately as #3872. Once that is merged, I can rebase this.

[rami3l]

Sounds like a good direction! I'm not completely clear on which order you want to merge things?

@djc Just the top-down order from 1 to 4. It's a back-and-forth dance around #2367, but the latter is a small change and I believe we can afford reverting and then reviving it.

PS: Just in case it's not clear:

• (2.) relies on subscribers being made possibly local in (1.)
• (3.) relies on implicit log subscribers from (2.)
• (4.) relies on the removal of with_runtime() in (3.)

It's a bit late now here so I'll probably continue my work tomorrow, by polishing (1.) (i.e. #3868) first.

[rami3l]

Nit: I guess the linter actually wants you to write _guard, see https://github.com/search?q=org%3Arust-lang%20_guard%3A&type=code for more examples across the org.

[rami3l]

Thanks a lot for the patch! I've left some minor questions/suggestions, but great improvement overall :D