Use HelpLink for errors/warnings that already have a URL

[rainersigwald]

We have several errors/warnings that have a URL in the text of the message. After #5488 (thanks, @jmezac!) we'll have a structured way to represent this that will eventually (dotnet/project-system#6335) be used in the Visual Studio UI.

We should use that!

Likely candidates:

$ rg "fwlink|aka\.ms" -g *resx
src\Tasks\Resources\Strings.resx
683:    <value>MSB3152: To enable 'Download prerequisites from the same location as my application' in the Prerequisites dialog box, you must download file '{0}' for item '{1}' to your local machine. For more information, see http://go.microsoft.com/fwlink/?LinkId=616018.</value>
1568:    <value>MSB3276: Found conflicts between different versions of the same dependent assembly. Please set the "AutoGenerateBindingRedirects" property to true in the project file. For more information, see http://go.microsoft.com/fwlink/?LinkId=294190.</value>
1971:    <value>MSB3474: The task "{0}" is not supported on the .NET Core version of MSBuild. Use the Microsoft XML Serializer Generator package instead. See https://go.microsoft.com/fwlink/?linkid=858594 for more information.</value>
2156:    <value>MSB3644: The reference assemblies for {0} were not found. To resolve this, install the Developer Pack (SDK/Targeting Pack) for this framework version or retarget your application. You can download .NET Framework Developer Packs at https://aka.ms/msbuild/developerpacks</value>
2545:    <value>MSB3783: Project "{0}" depends upon SDK "{1} v{2}" which was released originally for apps targeting "{3} {4}". To verify whether "{1} v{2}" is compatible with "{5} {6}", contact the SDK author or see http://go.microsoft.com/fwlink/?LinkID=309181.</value>
2553:    <value>MSB3842: Project "{0}" depends upon SDK "{1} v{2}" which supports apps targeting "{3} {4}". To verify whether "{1} v{2}" is compatible with "{5} {6}", contact the SDK author or see http://go.microsoft.com/fwlink/?LinkID=309181.</value>
2730:    <value>MSB4803: The task "{0}" is not supported on the .NET Core version of MSBuild. Please use the .NET Framework version of MSBuild. See https://aka.ms/msbuild/MSB4803 for further details.</value>

src\MSBuild\Resources\Strings.resx
791:    <value>For more detailed information, see https://aka.ms/msbuild/docs</value>

src\Build\Resources\Strings.resx
1197:    <value>Project file contains ToolsVersion="{0}". This toolset may be unknown or missing, in which case you may be able to resolve this by installing the appropriate version of MSBuild, or the build may have been forced to a particular ToolsVersion for policy reasons. Treating the project as if it had ToolsVersion="{1}". For more information, please see http://go.microsoft.com/fwlink/?LinkId=293424.</value>

[PeteBoyRocket]

@rainersigwald I am interested in doing this if its available. I have not contributed before so any pointers on what I need to do to get involved is welcome!

Looking at the code, this enhancement looks fairly straightforward for most cases. How were you imagining storing the urls? For example we currently have:

  <!-- Some tasks are only supported on .NET Framework -->
  <data name="TaskRequiresFrameworkFailure" xml:space="preserve">
    <value>MSB4803: The task "{0}" is not supported on the .NET Core version of MSBuild. Please use the .NET Framework version of MSBuild. See https://aka.ms/msbuild/MSB4803 for further details.</value>
    <comment>{StrBegin="MSB4803: "}</comment>
  </data>

Would I just add it as another resource following some naming convention like:

  <data name="TaskRequiresFrameworkFailure.HelpLink" xml:space="preserve">
    <value>https://aka.ms/msbuild/MSB4803</value>
  </data>

[vijaya-lakshmi-venkatraman]

Hi,
Is this still available?

[KalleOlaviNiemitalo]

I think the URLs can be just string literals in the source code. Because HTTP server can redirect the browser to a localised article, MSBuild can use the same URLs for all languages and need not place them in string resources where they could be localised.

[rainersigwald]

@rainersigwald I am interested in doing this if its available. I have not contributed before so any pointers on what I need to do to get involved is welcome!

@PeteBoyRocket sorry! I was out on parental leave when you posted and it looks like the team missed this :( Since that was two years ago I'm going to assume you've moved on, as only makes sense . . .

@vijaya-lakshmi-venkatraman Yes, I'll assign this to you. As @KalleOlaviNiemitalo mentioned, the URLs can be hardcoded in source, as long as they're fwlink or aka.ms links; Microsoft employees can tweak the backend that supports those links to get them to point where they need to go.

[vijaya-lakshmi-venkatraman]

So, the change should be just changing the value tag as below & removing the comment tag?
<value>https://aka.ms/msbuild/MSB4803</value>

[KalleOlaviNiemitalo]

msbuild/src/Tasks/LC.cs

Lines 102 to 106 in ef92093

	public override bool Execute()
	{
	Log.LogErrorWithCodeFromResources("TaskRequiresFrameworkFailure", nameof(LC));
	return false;
	}

LogErrorWithCodeFromResources could be overloaded with a method that takes a string helpLink parameter, and then the URL could be specified in this call. However, that would have a high risk of treating some argument as a URL when it's actually intended to go in params string[] messageArgs, thus a source breaking change, and it cannot be fixed by changing the order of parameters as in #5572 because the types of the parameters are the same. For that reason, I think it would be better to add the helpLink parameter only to the higher-arity method, and then add an internal extension method with a different name to simplify the calls. (Changing the type of the parameter to System.Uri could be another way but it would not be consistent with existing methods.)

msbuild/src/Shared/TaskLoggingHelper.cs

Lines 840 to 850 in 65c50fb

	public void LogErrorWithCodeFromResources
	(
	string subcategoryResourceName,
	string file,
	int lineNumber,
	int columnNumber,
	int endLineNumber,
	int endColumnNumber,
	string messageResourceName,
	params object[] messageArgs
	)

msbuild/src/Shared/TaskLoggingHelper.cs

Lines 670 to 683 in 2db11c2

	public void LogError
	(
	string subcategory,
	string errorCode,
	string helpKeyword,
	string helpLink,
	string file,
	int lineNumber,
	int columnNumber,
	int endLineNumber,
	int endColumnNumber,
	string message,
	params object[] messageArgs
	)

If there is concern about duplicating the same URLs in the source code of many tasks, then they can be defined as const strings in some internal class.

Another possible strategy would be to change LogErrorWithCodeFromResources to extract the help link from the resource string, too. This would have a higher risk of breaking compatibility, if some third-party tasks call this method and have URLs in their resources but don't want to use them as help strings. Also, it would require keeping the help links in the resource data and formatting them in a consistent way (e.g. always surrounded with <…>) so that they can be reliably detected. The implementation could fit in ResourceUtilities. I think the help link should be extracted before the format arguments are plugged in, rather than after, so that it isn't mislead by format arguments that are URLs but not intended as help links (e.g. if the error is that the resource at the URL cannot be accessed).

By the way, I assumed that LogErrorFromException would read Exception.HelpLink, but it doesn't seem to do that.

[rainersigwald]

I agree with your analysis @KalleOlaviNiemitalo though I think I lean toward "make a new method named something like LogErrorWithCodeAndHelpLink" that explicitly calls the new LogError overload that passes along HelpLink.

Extracting a URL from the resource sounds nice but a) I'm not sure of the format we'd want to enforce--as you mention should it be <...> or just search for any http...-- and b) do we want the URL in the text part of the message, always, or should it just be in HelpLink?

[KalleOlaviNiemitalo]

If the help link were in the resource string, I think it should be near the code, which is likewise extracted and removed automatically:

  <data name="TaskRequiresFrameworkFailure" xml:space="preserve">
    <value>MSB4803 &lt;https://aka.ms/msbuild/MSB4803&gt;: The task "{0}" is not supported on the .NET Core version of MSBuild. Please use the .NET Framework version of MSBuild.</value>
    <comment>{StrBegin="MSB4803 &lt;https://aka.ms/msbuild/MSB4803&gt;: "}</comment>
  </data>

The author of the message could then decide whether to repeat the URI in the human-readable part of the string and how to format it there.

[rainersigwald]

From scratch I like that approach but I think it'd break existing parsing approaches (like CanonicalError). And since ideally the link doesn't need to be localized, I think I prefer separate storage.

[KalleOlaviNiemitalo]

Does CanonicalError parse the resource strings? I thought the resources were used only with LogErrorWithCodeFromResources and similar.

[rainersigwald]

That's true, unless you run MSBuild in an Exec task (shudder) or (more common) parse MSBuild's messages using your own similar regex, like actions/setup-dotnet does.

[KalleOlaviNiemitalo]

How would the resource strings be written to stderr bypassing LogErrorWithCodeFromResources and ResourceUtilities, which would remove the prefix?

msbuild/src/Shared/ResourceUtilities.cs

Lines 122 to 125 in 2db11c2

	if (i < message.Length)
	{
	message = message.Substring(i, message.Length - i);
	}