[Xamarin.Android.Build.Tasks] Improve error text

[brendanzagaeski]

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=43245

I wouldn't be surprised if this download mechanism gets redesigned at
some point in the future. That could make these suggested changes
obsolete. But I suspect the current download mechanism will be around
for at least a little while longer, so adding these corrections and
improvements will still be helpful.

• ErrorMessage():
• Check for InstallInstructions first. If custom instructions
  are available via that property, they should take precedence
  over the more generic message for PackageName.
• Rewrite the message for PackageName. The old error was
  incorrect. (For example, there was no "SDK installer" that
  would help resolve the error.) Additionally the new if
  statement in AddAttributeValue() removes some redundant
  appearances of this error, so the message can now provide more
  specific instructions.
• AddAttributeValue():
• Add if (sourceUrl.Name == null) to avoid logging unnecessary
  additional errors. MakeSureLibraryIsInPlace() now provides
  appropriate errors for each possible failure when
  sourceUrl.Name != null, so additional errors are only needed
  if sourceUrl.Name == null.
• ExtractArchive():
• Update the error message to account for the MD5 file names and
  the "zips" folder that were added in July 2015 (monodroid/master
  e8737f15c5e6f71f01f566475865a2cb34bfdfc9). Also remove the
  phrase "Please download" because re-downloading will not
  necessarily help with problems unzipping the file.
• MakeSureLibraryIsInPlace():
• Update the error message to account for the MD5 file names and
  the "zips" folder that were added in July 2015 (monodroid/master
  e8737f15c5e6f71f01f566475865a2cb34bfdfc9).
• Add a new explicit error with code "XA5217" for the case where a
  .zip file is extracted without error but is missing an expected
  nested archive. The old logic allowed the build process to
  continue even if all 3 attempts failed, and that could lead to
  some confusing error messages later in the build.
• Execute():
• Add parentheses and "Details:" to the second part of each error
  message because the file name of the missing item is usually
  much less important than the first part of the error message.

[xamarin-release-manager]

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

[dnfclas]

Hi @brendanzagaeski, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, DNFBOT;

[jonpryor]

Why the addition of (Details: and )?

This might make sense if XA5207 were emitted twice, and the "(Details:" message was the second message, providing additional information for the first message, but this isn't the case; XA5207 is only emitted once per "logical construct" (assembly/etc.).

[brendanzagaeski]

Yeah, I debated with myself on the best option. I think it could also be OK to log an additional separate XA5207 that lists the missing file name, but I wasn't sure about the conventions for when or when not to log 2 separate messages for one error code.

One thing I forgot to check was whether there was any precedent for using something similar to these parentheses and "Details:" text in other error messages. If not, then another approach might indeed be better. The issue I was aiming to address is that the error messages are a bit more confusing when there isn't a clear separation between the first part of the message and the details about the missing file.

For example, compare:

error XA5207: Unable to restore Java resources for package 'Foo'. The authors of the package should set the SourceUrl property or InstallInstructions property on the Java.Interop.JavaLibraryReferenceAttribute. (Details: Java library file foo.jar doesn't exist.)

To:

error XA5207: Unable to restore Java resources for package 'Foo'. The authors of the package should set the SourceUrl property or InstallInstructions property on the Java.Interop.JavaLibraryReferenceAttribute. Java library file foo.jar doesn't exist.

Or compare:

error XA5207: Installation instructions: Please download foo.jar from http://example.com/foo.jar and copy it into the Android app project directory. (Details: Java library file foo.jar doesn't exist.)

to:

error XA5207: Installation instructions: Please download foo.jar from http://example.com/foo.jar and copy it into the Android app project directory. Java library file foo.jar doesn't exist.

[brendanzagaeski]

Thinking about it now, I realized that a quick way to check for other uses of parentheses in error messages was to look at the list in https://github.com/xamarin/java.interop/blob/master/src/Java.Interop.Tools.Diagnostics/Java.Interop.Tools.Diagnostics/Diagnostic.cs. Indeed there are not any other error messages that match this idea of using "(Details: ...)".

So maybe it would be best to split the specific file information into its own error line, if that wouldn't break convention?

Example:

error XA5207: Unable to restore Java resources for package 'Foo'. The authors of the package should set the SourceUrl property or InstallInstructions property on the Java.Interop.JavaLibraryReferenceAttribute.

error XA5207: Java library file foo.jar doesn't exist.

[jonpryor]

All the messages are not great. :-)

The problem is twofold: People don't read messages, and the IDE will truncate messages. We should thus move the "useful" data to the beginning:

error XA5207: Could not find foo.jar while restoring resources for package Foo. The authors of the package should set the SourceUrl or InstallInstructions property on Java.Interop.JavaLibraryReferenceAttribute.

This moves the relevant/important message to the front, and the suggestion for package authors to the end. It's also shorter (somewhat...).

So maybe it would be best to split the specific file information into its own error line, if that wouldn't break convention?

That could be better (theoretically), but I don't see how that's an improvement in this circumstance.

[brendanzagaeski]

Cool. Moving the file name to the beginning along with the package name sounds good to me. I'll try rearranging the error messages like that and I'll update the pull request.

[dellis1972]

@brendanzagaeski this looks good.
Just a heads up the PR #150 will tackle allot of the partial download issues. But I think improving the error messages is good too.

[jonpryor]

What is the purpose of an error message? It's not (necessarily) just to say "something went wrong." If that were the sole purpose, we could print out the Reason message and be done with it.

The purpose of the error message is also to inform the user of what the problem is in such a way that they can fix it.

Your updated message doesn't satisfy that purpose. We just know that unzipping some file failed for some reason. We don't know what to do to fix it. Should we clean and rebuild the project? Sacrifice a chicken? Burn the world? Who knows! The error message certainly doesn't say....

As such, the previous message is much better.

[brendanzagaeski]

Here are a few things I was considering while working on this "first draft" of the update for this error message:

• The instructions in the original message were incorrect after monodroid/master e8737f15c5e6f71f01f566475865a2cb34bfdfc9. The output location for unzipping the file by hand was no longer correct. Additionally, the new logic expects the downloaded .zip file to exist in the "zips" folder. So the user should now complete at least 3 steps for the "fully manual" procedure: download the file, copy it to the MD5 file name in the "zips" folder, and then extract the main archive.
• At the time when this error happens, "some" version of the file does already exist. The additional "Reason:" information helps clarify how the unzipping failed. For example, the problem could be a permissions or I/O error (such as unable to write to a full disk). In those cases, the user should look carefully at the additional "Reason" to see if re-downloading the file is likely to help.
• If this error occurs when unzipping one of the embedded archives rather than the main outer archive, then there is no way to re-download just the embedded archive. Additional conditional logic and lengthier instructions would be needed to account for that scenario. (This could happen due to a disk-full error during unzipping or maybe more likely a "path too long" exception on Windows.)

That said, I'd be happy to compromise between the old and the new error message. In the particular case where the "Reason" is "File ... is not a ZIP archive", the error does most likely indicate an incomplete download (at least for the Xamarin.Android.Support packages). And that scenario is arguably the most common reason for a failed unzip. So I'd be happy to either:

(a) Add some conditional logic to check whether the "Reason" is "not a ZIP archive" and include an additional recommendation about re-downloading the file in that case. It appears that #150 now implements that approach during the download step itself, so perhaps we don't have to worry about it anymore during the unzipping step.

or

(b) Include the re-downloading recommendation directly in the message with some "conditional wording." Something like:

("Unzipping failed. Please check the "Reason:" for additional details. If the reason is "not a ZIP archive", then the file download might have been incomplete, so a good first step would be to try re-downloading {0} by hand and copying it to {1}", url, file)

[brendanzagaeski]

After considering this a little more, I think depending on #150 to provide the "not a ZIP" message is indeed probably the simplest approach. That might also mean that the 2 separate "XA5209" lines can now be condensed into just 1 line with the "Reason:" at the end (there is some precedent for that).

I'll aim to add a "second draft" onto this pull request by Monday that incorporates the various notes so far. Thanks!

[monojenkins]

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

[monojenkins]

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

[jonpryor]

Closing as:

1. This PR hasn't been touched or updated in 8 months, and
2. This PR is no longer mergeable, and
3. The new error messages are not unambiguously clearer or better