Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance msb4064 unexpected task attribute error #5945

Conversation

@BartoszKlonowski
Copy link
Contributor

@BartoszKlonowski BartoszKlonowski commented Dec 7, 2020

This pull request fixes #3922.

The MSB4064 error has been provided with additional information about the assembly such as it's version, full name, etc.


Changes provided within this commit contains:

  • modify the MSB4064 string in resources,
  • adjust it's usage in the code throwing this error,
  • run the default translations by compilation

Results:

The example of MSB4064 printed is presented below:
obraz

Copy link
Contributor

@rainersigwald rainersigwald left a comment

This looks great, but I have a vague recollection that it isn't enough. I'll try to figure out why I think that and get back to you. Hopefully I can convince myself that past-me was just wrong!

@BartoszKlonowski
Copy link
Contributor Author

@BartoszKlonowski BartoszKlonowski commented Dec 8, 2020

@rainersigwald Perhaps what you feel that is missing here is a path (on local disk) to the assembly?
Thing is, that in my first approach to this, I've tried to use:

_taskFactoryWrapper.TaskFactoryLoadedType.Assembly.AssemblyLocation

but when debugging it, this path has always been null.
So due to the fact that I couldn't find any better path-containing property/variable, I decided to not include the path in the error message, so there's no confusion in case of that path didn't really work for some user.
I've tried to avoid the possible situation, where the error message would just display the empty space instead of correct path.

@rainersigwald
Copy link
Contributor

@rainersigwald rainersigwald commented Dec 16, 2020

Perhaps what you feel that is missing here is a path (on local disk) to the assembly?
Thing is, that in my first approach to this, I've tried to use:

_taskFactoryWrapper.TaskFactoryLoadedType.Assembly.AssemblyLocation

but when debugging it, this path has always been null.
So due to the fact that I couldn't find any better path-containing property/variable, I decided to not include the path in the error message, so there's no confusion in case of that path didn't really work for some user.

This seems pretty plausible! It would definitely be nice to have the path (I'd strongly prefer that over assembly identity) but I agree with your tradeoffs, and this is better than nothing.

Is that path null when you try with the minimal repro project I (just) shared in #3922 (comment)?

@BartoszKlonowski
Copy link
Contributor Author

@BartoszKlonowski BartoszKlonowski commented Jan 6, 2021

@rainersigwald Answering your question: no, the path isn't null when reproducing the error with the example you've provided me with.
I've pushed the changes and updated the PR's description - please check.

@Forgind
Copy link
Member

@Forgind Forgind commented Jan 7, 2021

It looks like LoadedAssembly is null for the ValidateNonExistantParameter (sic) test. It looks artificial to me, but are there any circumstances when it would be executing a task without loading an assembly? If so, maybe add a second error possibility depending on whether that exists?

@cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Jan 14, 2021

Looking through the code, LoadedType.LoadedAssembly is always checked for null, so apparently it can be null. My assumption is that tasks that are not loaded from assemblies have it set to null (like inline tasks for example). So you'll have to treat the null case somehow. Most basic is to revert to the old message if it's null. Another implementation is to revert to another message that writes out the task factory type, which hopefully should be enough to let the user guess where that task is coming from. The common case is to have assembly based tasks.

Regarding tests, you have to update the mocked types to have an assembly, probably over here:

TypeLoader typeLoader = new TypeLoader(IsTaskFactoryClass);
#if !FEATURE_ASSEMBLYLOADCONTEXT
AssemblyLoadInfo loadInfo = AssemblyLoadInfo.Create(Assembly.GetAssembly(typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory)).FullName, null);
#else
AssemblyLoadInfo loadInfo = AssemblyLoadInfo.Create(typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory).GetTypeInfo().FullName, null);
#endif
LoadedType loadedType = new LoadedType(typeof(TaskBuilderTestTask.TaskBuilderTestTaskFactory), loadInfo);

Then you need to write two tests: one when the assembly is null, and one when the assembly is not null, and assert the proper error message in both.

@Forgind
Copy link
Member

@Forgind Forgind commented Feb 1, 2021

@BartoszKlonowski, status update?

@BartoszKlonowski
Copy link
Contributor Author

@BartoszKlonowski BartoszKlonowski commented Feb 1, 2021

@Forgind Thanks for asking, and sorry for such a long delay...
No update from my side - Unfortunatelly I have no time to work on this.
If you're asking because you'd like to take it, then let me know if you have any questions. Otherwise, I'll try to push that forward by the end of this week.
Let me know.

@Forgind
Copy link
Member

@Forgind Forgind commented Feb 1, 2021

No worries! If you didn't want to work on it anymore, I was willing to try to get it ship-shape, since I think it's pretty close. End of this week (or end of next week) is fine. Thanks!

This commit enhances the MSB4064 error with additional information and
details such as:
 - assembly identity - it's full name containing the token ID, version
   and name.
The enhanced MSB4064 error message has previously been implemented
without assembly location. It has been added by this commit.
Note that assembly parameter used for the error message has been
replaced with loadedAssembly parameter, due to having the null name
property.
@BartoszKlonowski BartoszKlonowski force-pushed the BartoszKlonowski:enhance-MSB4064-unexpected-task-attribute-error branch from cd658dd to 3ba346a Feb 6, 2021
It turns out, that when executing the tests, the LoadedAssembly is null
which breaks the run.
To avoid such situation, checking for null has been implemented, so in
case of no assembly loaded the type is checked and it's path and name is
returned to the user/developer.
@Forgind
Forgind approved these changes Feb 6, 2021
Copy link
Member

@Forgind Forgind left a comment

LGTM! Thank you!

@ladipro
ladipro approved these changes Feb 8, 2021
Copy link
Contributor

@ladipro ladipro left a comment

Looks great! One small nit inline.

@@ -1225,7 +1225,7 @@
<comment>{StrBegin="MSB4091: "}</comment>
</data>
<data name="UnexpectedTaskAttribute" xml:space="preserve">
<value>MSB4064: The "{0}" parameter is not supported by the "{1}" task. Verify the parameter exists on the task, and it is a settable public instance property.</value>
<value>MSB4064: The "{0}" parameter is not supported by the "{1}" task loaded from assembly: {2} from the path: {3}. Verify that the parameter exists on the task, the UsingTask points to the correct assembly and it is a settable public instance property.</value>

This comment has been minimized.

@ladipro

ladipro Feb 8, 2021
Contributor

super-nit: In many other error messages mentioning UsingTask I see it wrapped in angle brackets making it clear that it refers to an XML element. Also, a comma before the last and would be more consistent with the style of the rest of this file.

@Forgind Forgind merged commit 1a0d8e8 into dotnet:master Feb 9, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210208.12 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
@Forgind
Copy link
Member

@Forgind Forgind commented Feb 9, 2021

Thanks @BartoszKlonowski!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants