Output illink messages without --verbose

[marek-safar]

We need to propagate msbuild verbose level (-verbosity:level option) into illink.

The proposed mapping between msbuild verbosity and MessageCategory

q[uiet] >= MessageCategory.Error (not sure about this one)
d[etailed] >= MessageCategory.Info
diag[nostic] >= MessageCategory.Diagnostic

This should be done for illink task only, the command line interface for illink should still use -v to print everything

@vitek-karas how do you plan to use warning? If we go with an explicit -nowarn option we would need to expose that on VS UI.

[vitek-karas]

Errors and Warnings should be reported always, even on quiet mode.

I wonder if we could somehow hook into the <NoWarn>, for example NuGet seems to be part of it as it's valid (and works) to specify <NoWarn>NU.... But this need investigation.

[marek-safar]

Errors and Warnings should be reported always, even on quiet mode.

I agree but that means we need a solution to suppress the warnings which also means to add UI to all VS-es for that. NuGet seems to allow that on package level which I think is not we want.

[vitek-karas]

Not sure about the UI for this, but it apparently can be specified globally as well: https://github.com/microsoft/msbuild/blob/a4878e187d0640ee79b7cdc12c640f68e28a326b/Directory.Build.props#L54

[vitek-karas]

This already has VS UI

Ideally linker would somehow integrate with that. We might even want to choose warning levels and so on. That said, we should definitely discuss this with whoever "owns" this UI/property to make sure we don't break something else with such approach.

[marek-safar]

Oh, I didn't know they hacked into compiler warnings. This looks like prior art for us then which we can "just" use if we validate that the NU* prefix is not embedded anywhere in VS.

[sbomer]

I don't think we should feed in the MSBuild verbosity anywhere - rather, we should just make sure that our task uses stdout/stderr in a reasonable way, and formats warnings/errors how MSBuild expects them when it's being run from the task.

ToolTask is already designed to assign reasonable MSBuild MessageImportance to various bits of output, in a way that makes sense with the default logger (which treats MessageImportance according to https://docs.microsoft.com/en-us/visualstudio/msbuild/obtaining-build-logs-with-msbuild?view=vs-2019#verbosity-settings). For us that means:

• Outputs that are formatted like warnings or errors (https://docs.microsoft.com/en-us/archive/blogs/msbuild/msbuild-visual-studio-aware-error-messages-and-message-formats) show up at any MSBuild verbosity (quiet/minimal/normal/detailed/diagnostic)
• The command-line invocation of the tool gets MSBuild MessageImportance.High, which shows up in minimal/normal/detailed/diagnostic
• Stderr gets MSBuild MessageImportance.Normal, which shows up in normal/detailed/diagnostic
• Stdout gets MSBuild MessageImportance.Low, which shows up in detailed/diagnostic

The importance of stdout/stderr can be changed if we prefer a different behavior. IMO we might want to make Stdout have MessageImportance.Normal, so that it is visible with "normal" verbosity (with the default importance of Low, it shows up only at detailed+, in a dark grey that is hard to read against a black background).

We should then make sure that stdout and stderr by default only show messages we care about, and an extra property (say TrimmerVerbosity) can tell the tool to include more output.

[sbomer]

Also, apparently MSBuild intentionally doesn't provide a way to get the current verbosity

[marek-safar]

Stderr gets MSBuild MessageImportance.Normal, which shows up in normal/detailed/diagnostic
Stdout gets MSBuild MessageImportance.Low, which shows up in detailed/diagnostic

This makes sense to me, we should tweak the msbuild output to respect that. Does it also mean the error/warning should go to stderr?

show messages we care about, and an extra property (say TrimmerVerbosity) can tell the tool to include more output.

Please don't, that would complicate things with no good reason

[vitek-karas]

I agree that we should not have any logging which doesn't fall directly into one of the MSBuild categories. If there's a need for a more detailed trace, we can introduce a separate solution which produces this information to a file. But the existing support for dependency tracing is kind of already that. We could always extend that if needed.

[sbomer]

Actually, I forgot that @tlakollo already changed the stderr importance to High in https://github.com/mono/linker/pull/719/files. So currently text logged to stderr will be visible at minimal+ (minimal is the default verbosity at least when building from the command-line, if it's not specified), and text logged to stdout will be visible at normal+.

Does it also mean the error/warning should go to stderr?

It doesn't matter for MSBuild - anything from stderr or stdout that is parsed as a warning/error will be treated as such independently (and it will only show up once in the output). I think it would make sense to log warnings/errors about "linker-unfriendly" code patterns to stdout, and internal errors, improper command-lines, malformed xml, diagnostics, etc. to stderr. That way when running from the command-line you can easily redirect warnings from code analysis to a separate file, while separately seeing tool errors/diagnostics output to console.

an extra property (say TrimmerVerbosity) can tell the tool to include more output.

This was just to say that if we have a "--verbose" flag that includes extra diagnostics or similar, there should be a corresponding way to turn that on from the task. To keep this simple, I think we shouldn't change the content of what we log based on whether we are running from MSBuild - we should only change the format of warnings/errors so they are properly picked up.

The way I imagine this working is that we would have something like the following helpers (or you could do it by defining a MessageKind or similar - I'm not trying to suggest the exact shape):

• LogMessage -> stdout
  • Used for the help message and other essential informational output.
• LogWarning -> stdout (formatted specially when run from MSBuild)
• LogError -> stdout (same, with MSBuild format)
• LogInternalError -> stderr (MSBuild format)
  • Any time we return a failure exit code, we should try to log some kind of internal error to indicate what happened.
• LogInternalMessage -> stderr
  • Here it might still be useful to have an internal "log level" or "verbosity" concept to control how much diagnostic info we spew to stderr. Or, we can keep it simple and only support dumping extra output to a separate file.
• Possibly LogInternalWarning -> stderr
  • If we have any internal warnings that should show as MSBuild warnings. I'm not sure this is needed at all.

[sbomer]

I looked into the suppression mechanism. The VS options from @vitek-karas's screenshot map to MSBuild properties NoWarn and WarningsAsErrors, which are just plain MSBuild properties, and every tool that wants to support them needs to do so independently.

There's also a new set of MSBuild properties like MSBuildWarningsAsMessages that will do the same for arbitrary warning codes, with special MSBuild support: dotnet/msbuild#1928. They don't have VS support, but should be usable from project files.

For the end-to-end experience, I suggest we rely on dotnet/msbuild#4421 which would make NoWarn and possibly WarningsAsErrors flow to the new properties, making them work out of the box for warnings defined by external tools. It looks like that issue is getting some traction but we should keep an eye on it.

[marek-safar]

I think it would make sense to log warnings/errors about "linker-unfriendly" code patterns to stdout, and internal errors, improper command-lines, malformed xml, diagnostics, etc. to stderr. That way when running from the command-line you can easily redirect warnings from code analysis to a separate file, while separately seeing tool errors/diagnostics output to console.

I don't think we should go this way all error and warnings including ones from custom steps should output the same way. The code analysis will most likely have its own switch if someone want to hide/show it they use the switch.

The way I imagine this working is that we would have something like the following helpers

We have now this and I think it's sufficient, don't see any good reason for adding LogInternalError. The question is if it's working as expected or anything else needs to be done to filter the output when someone uses -v:n or -v:diag.

[vitek-karas]

@sbomer Regarding the msbuild integration - if we can get the required functionality it would work great. The question is if we can get that funded and in time. Can you please reach out to the msbuild owners to figure it out?

[sbomer]

all error and warnings including ones from custom steps should output the same way

In that case, the existing CreateErrorMessage and CreateWarningMessage would be enough. Presumably they would both go to stdout?

The code analysis will most likely have its own switch if someone want to hide/show it they use the switch.

That sounds good, as long as the code analysis warnings, when they are on, are formatted as MSBuild warnings (presumably using CreateWarningMessage).

Would CreateInfoMessage and CreateDiagnosticMessage go to different output streams stdout/stderr? If they go to the same place, then they will get the same MSBuild verbosity.

if it's working as expected or anything else needs to be done to filter the output when someone uses -v:n or -v:diag

There's no supported way to get the current MSBuild verbosity from a task, so we can't exactly filter based on the MSBuild verbosity. We can filter based on our own verbosity flags, and we can decide what MessageImportance our stdout/stderr get.

I think all we need to do here is to decide which messages go to stdout/stderr, and decide on a MessageImportance for each stream.

[sbomer]

I changed the title to reflect the remaining work. @mateoatr's work on the new diagnostic infrastructure means that the warnings/errors are getting the right MSBuild importance, and there is a mechanism (--nowarn) to selectively disable them. All that remains is to:

• Remove the --verbose requirement to emit these warnings
• Decide how we will suppress various warnings in the SDK (either NoWarn or warning versions)

[vitek-karas]

Decide how we will suppress various warnings in the SDK (either NoWarn or warning versions)

I think we need solutions for two scenarios here:

• Way to by default suppress all warnings when building .NET Core 3.* app - ideally this would be done via warning versions if we can fit this in
• Way to suppress trim analysis warnings by default (for all apps, but mainly .NET 5 as .NET 3.* should not produce warnings as per above) - I think this should be done on the SDK side by adding the trim analysis warnings to NoWarn by default (and linker will pick it up from there). We just need a name of a new property to opt-in to trim analysis warnings. - basically Introduce option to enable trim analysis warnings sdk#12388.

[sbomer]

by default suppress all warnings when building .NET Core 3.* app - ideally this would be done via warning versions if we can fit this in

Agreed, hopefully we can fit it in.

suppress trim analysis warnings by default (for all apps, but mainly .NET 5 as .NET 3.* should not produce warnings as per above) - I think this should be done on the SDK side by adding the trim analysis warnings to NoWarn by default

Why do you suggest NoWarn instead of versions to disable them for .NET5? Using versions would let us use just one property to disable all of them for 3.0, disable most for 5.0, allow devs to opt in in 5.0, and turn them on by default in a future version.

The argument I can see is that the "trim analysis" should be a separate category rather than a version. I do think it makes sense to have independent controls "version" and "analysis kind" (or "category") for warning behavior, but we have been going back and forth on the categories for a bit now. In my mind the definition of the "trim analysis" warning set should ultimately live in illink.dll, not in the SDK; using NoWarn in the SDK for 5.0 was just a way to get around our hesitance to introduce categories. Even when we do introduce categories, the "trim analysis warnings" would be versioned like any other, so if we plan to support warning versions in this release, it seems preferable to NoWarn. Thoughts?

[vitek-karas]

My reading/understanding of the warning level as designed for Roslyn is that it's not about default experiences for particular application, it's about warning behavior across major versions of the product (making sure that new SDKs don't break old apps and make this explicit).

The fact that we disable trim analysis warnings by default now is a point-in-time decision based on the state of the product. In theory we may enable some of these in later 5.* release (very unlikely, but that's not the point). Trim analysis warnings will essentially work the same way in .NET 5 and in .NET 6 (we just hope that framework and other code will be more ready for it).

This is similar to nullability warnings in Roslyn - I don't think warning level has anything specific for nullability - enabling or disabling nullability warnings is orthogonal to warning level. For example in .NET 5 all of framework is nullable-annotated (unlike 3.1) - does that mean we should enable nullability warnings for everything in .NET 5 apps (including application code)? Definitely no - but that has no tie to warning version - at least for me.

I know we've been struggling with the analysis warning categorization - but to me that's a completely distinct problem from warning levels - tying them together is just "an easy way out" which I fear will hurt us in the future.

In a way I agree that analysis warning categorization sort of belongs to the linker itself, but in reality it doesn't matter - the warnings are essentially part of public API and thus are "set in stone" - as in "Does IL2047 belong to 'trim correctness analysis' category?" will have the same answer in .NET 5 as well as in .NET 6 and any future release. So I'm fine if it's easier for us to currently encode this knowledge into the SDK and later on (in .NET 6 for example) we could move this down to the linker if that makes more sense.

Additionally if we hide trim analysis warnings behind warning version as per the above we can run into this:

• Somebody in .NET 5 app will want to enable the trim analysis warnings - so they set warning version to 9999 (as that would be the only way to do that) - note that setting warning version to 9999 is not done here because "I want to see all warnings always ever", it's done because "I want to see trim analysis warnings, but I have no other way to describe this intent".
• We ship .NET 6 which will also introduce Single-File analysis warnings
• Compiling the above .NET 5 app with .NET 6 SDK will now start reporting Single-File analysis warnings - which directly beats the purpose of warning versions

[sbomer]

it's about warning behavior across major versions of the product (making sure that new SDKs don't break old apps and make this explicit).
We ship .NET 6 which will also introduce Single-File analysis warnings

I think we fundamentally agree about the purpose of versions, and that single-file for example needs to be covered by an additional category, not just versions - I am not suggesting to use versions in place of categories. Since we only have the one category of warnings related to trimming in .NET5, and since we haven't designed the categories, effectively what I'm suggesting is to treat the defaults as "trim analysis is on, with only a subset of the warnings enabled in v5 (including warnings about incorrectly used attributes, but not RequiresUnreferencedCodeAttribute, for example)".

Treating the defaults as "trim analysis is off in .NET5, but you can turn all trim analysis warnings on with a flag" makes sense too. I am also ok with temporarily defining that category in the SDK. I think the discussion then hinges on what belongs in the trim analysis category. I opened a new issue to discuss that: #1367