Request silent restore for all separate restore steps

[JanKrivanek]

Context

Separate execution of restore for some of the invoked msbuild commands lead to suboptimal experience with terminal logger feature. Example: dotnet/msbuild#9614

Changes made

This PR is proposing:

• Each case of additionl restore step should be invoked in quiet mode for terminal logger (-tlp:verbosity=quiet)
• The Get<X> commands still lead to quiet verbosity for all loggers (-verbosity:quiet)
• Workloads advertising is OK - unless user didn't issue msbuild get<X> commands (this stays untouched from current main behavior)
• If user explicitly specifies verbosity, the quietness of the separate restore step is not forced (this is debatable, but less risky for now)

Testing

Existing unit tests + manual testing of the case from the user filed issue

[Forgind]

I'm thinking we might want to make this a bit more specific. It looks like you're changing it from "be quiet if getX is specified" to "be quiet for separate restore," but it sounds like this is only something we'd want for tl because it hides and rearranges information, so if you have tl disabled, I'd expect current behavior (be quiet only if getX is specified).

[baronfel]

so if you have tl disabled, I'd expect current behavior

This is hard to discover in the SDK because the feature-detection for TL happens internal to MSBuild.

[JanKrivanek]

I guess we might use the new ability of specifying verbosity just for terminal logger added by @AR-May: dotnet/msbuild#9810 (comment)

If I understand the feature right, then adding -tlp:v=quiet instead of -verbosity:quiet, will make sure this applies only for terminal logger case.
For getX cases, we'll still keep passing -verbosity:quiet

Thoughts?

[baronfel]

That seems like a great resolution, good thinking.

[Forgind]

I agree; that sounds perfect to me 🙂

[JanKrivanek]

Good.

So updated behavior:

• nologo for any separate restore invocation (it'll be anyways disaplayed by subsequent msbuild invocation) - as this is hard to tailor for tl only
• -verbosity:quiet added for getX commands only
• -tlp:verbosity=quiet added for all cases leading to separate restore - this is though ignored unless terminal logger is used

[Forgind]

I think that if nologo is applied to one dotnet command, it still creates the first use sentinel, which means the subsequent command would also not print a logo. Is that correct @baronfel? If so, is that something we should change?

[JanKrivanek]

I believe msbuild nologo is slightly different in this.

But btw. I've just realized I was recently removing the version string in case terminal logger is used :-) dotnet/msbuild#9831 - so it's actualy superfluous in case of tl. So removing this as well...

[baronfel]

I think we need to keep nologo at minimum for cases where TL isn't used - the SDK doesn't do TL probing itself and so cannot know if the presence/absence of TL will influence of the logo is shown.

[JanKrivanek]

I think we need to keep nologo at minimum for cases where TL isn't used - the SDK doesn't do TL probing itself and so cannot know if the presence/absence of TL will influence of the logo is shown.

Yes - I finally kept that behavior unchanged - -nologo is added IFF get<X> build parameter is used.
The description of this PR should be up to date in the intended behavior.

Let me know if you have any concerns with the suggested behavior.

[Forgind]

Thanks for helping us get a good design! The only part currently confusing me is that on line 64, it says we should only add these arguments if (arguments != null). I think that if arguments is null, we would still want -tlp:verbosity=quiet. I don't think it matters, though, because I think arguments is never null...but then I'm curious why that check exists in the first place...

[JanKrivanek]

Thanks for helping us get a good design! The only part currently confusing me is that on line 64, it says we should only add these arguments if (arguments != null). I think that if arguments is null, we would still want -tlp:verbosity=quiet. I don't think it matters, though, because I think arguments is never null...but then I'm curious why that check exists in the first place...

Very good point!
No reason not to have that code more solid - I'm updating that