Allow an Override Flag in UsingTasks by benvillalobos · Pull Request #6783 · dotnet/msbuild

benvillalobos · 2021-08-23T23:20:19Z

Context

When we define multiple usingtasks like so:

  <UsingTask TaskName="VSCTCompiler" AssemblyFile="$(VsSDKAssemblyFile)" />
  <UsingTask TaskName="VSCTCompiler" AssemblyFile="$(VsSDKAssemblyFile)" Architecture="x86" />

and call the task without specifying Architecture, the architecture-specific UsingTask should be returned when MSBuild searches for the task. Currently, MSBuild prefers the most "exact match".

This change allows the following:

  <UsingTask TaskName="VSCTCompiler" AssemblyFile="$(VsSDKAssemblyFile)" />
  <UsingTask TaskName="VSCTCompiler" AssemblyFile="$(VsSDKAssemblyFile)" Override="true" Architecture="x86" />

Users can specify Override="true" in a UsingTask element. This will modify the TaskRegistry to always return that task registration when attempting to call that task.

Changes Made

The TaskRegistry will return the first usingtask marked with Override="true" when searching for a task.

Testing

A test showing that:

Multiple usingtasks overriding the same task will log MSB4275
If a usingtask overrides, it is preferred over other usingtasks.

Notes

Don't review commit by commit.

Expected behaviors

If multiple overrides override a task with the exact same name (fully qualified), MSB4275 will be logged

Prioritization order of usingtasks:

Project-level usingtask override
.tasks/.overridetasks file
UsingTask defined elsewhere

benvillalobos · 2021-08-23T23:35:13Z

Thinking out loud, is #5541 more about when a task has Architecture defined? Maybe we can add a check that prioritizes the usingtask that has Architecture (and any other "high priority" attributes) defined when trying to find the task from the registry?

rainersigwald · 2021-08-24T03:08:05Z

Thinking out loud, is #5541 more about when a task has Architecture defined?

I think so; the question is "given an old poorly-annotated x86 task + target buried in a NuGet package or old SDK or something, is there something a user project can do to fix the build?" With other things (like targets or properties or items) you can override behavior if you can get the ordering right. But here you can't.

Maybe we can add a check that prioritizes the usingtask that has Architecture (and any other "high priority" attributes) defined when trying to find the task from the registry?

I think so? Architecture is the only one we care about right now, maybe someday we'll want core/framework.

benvillalobos · 2021-08-31T19:27:00Z

Current thought for this PR is to have some Dictionary containing <Name of task that has an Architecture-specific usingtask, The registration record for that task>. And I'll short-circuit the task lookup method when the task we're looking for happens to have an architecture-specific version.

benvillalobos · 2021-08-31T22:08:11Z

NTS: I didn't consider there potentially being multiple usingtasks that define Architecture but I haven't gotten to the selection process for preferred usingtasks yet. GenerateResource so far has multiple usingtasks.

benvillalobos · 2021-09-01T18:01:48Z

NTS: I didn't consider there potentially being multiple usingtasks that define Architecture but I haven't gotten to the selection process for preferred usingtasks yet. GenerateResource so far has multiple usingtasks.

From standup today, if multiple UsingTasks define Architecture, first one wins.

benvillalobos · 2021-09-01T18:51:57Z

NTS: But what about different tasks with the same name? Might need to check the dll its coming from as well.

benvillalobos · 2021-09-01T23:49:39Z

Because this will be modifying behavior of UsingTasks, we should update https://docs.microsoft.com/en-us/visualstudio/msbuild/usingtask-element-msbuild?view=vs-2019 when this merges.

benvillalobos · 2021-09-02T01:44:19Z

NTS: Need to modify tests in such a way that account for any usingtask with Architecture defined is prioritized. The problem is many tests check for Architecture/Runtime when doing their tests. Might be able to get away without the bool "hack".

benvillalobos · 2021-11-02T21:17:32Z

Notes after chatting with @rainersigwald

Keep in mind that we'll want to generalize this for Runtime as a parameter as well.

The arch/runtime should match the current arch/runtime.

Final thoughts we came to: A usingtask should specify some Override="true" to opt into this. If that's the case, it should override ANY call to a task with that name. Even if there are two tasks with the same name from two separate assemblies. At that point a user can call Namespace.Class.Foo to call the specific task they want.

We were looking at a project with the following usingtasks:

  
<UsingTask TaskName="Foo" AssemblyFile="$(Outdir)task.dll" Architecture="x86" />
<UsingTask TaskName="Foo" AssemblyFile="$(Outdir)task.dll"/>
<UsingTask TaskName="Foo" AssemblyFile="$(Outdir)task234.dll"/>
<!-- Problem: If Foo is called, the second declaration here will be 
       found on "Exact" match before fuzzy search happens. -->

  <Target Name="Foo" AfterTargets="Build">
    <Foo/>
  </Target>

The first UsingTask will be stored in a dictionary of <(TaskName, TaskIdentity), List>.

High level ordering of relevant functions when finding a task with a given name:

FindTaskInRegistry <- First attempts exact match then fuzzy
- GetRegisteredTask
  - GetTaskRegistrationRecord
    - // If fuzzy match
      - GetRelevantRegistrations <- Returns Dictionary<TaskIdentity, List>
        
        for each registration
        
        GetMatchingRegistration <- Finds the first partial match

We're thinking the bulk of the change should be made in GetRelevantRegistrations or RegisterTask.

Options if we modify GetRelevantRegistrations:

Sort usingtasks based on this Override flag, always return that one.
Store tasks marked as Override in a separate collection, short circuit this method if the task being searched for has an Override usingtask defined.

Options if we modify RegisterTask:

If some task Foo has Override specified, delete or modify existing entries of the Foo task to specifically use this usingtask.

benvillalobos · 2021-11-08T23:37:47Z

Note: I moved this check down 1 layer from GetRegisteredTask to GetTaskRegistrationRecord since the former calls the latter no matter what.

Unit tests also call directly into the latter.

baronfel

The proposed design makes sense to me, a few questions I had are:

would a user ever want to know about this decision being made? if so, should there be a reason why the decision was made? e.g a OverrideReason attribute where inline doc could be added, so we could add a MS1000000: Task 'TaskName' from assembly 'AssemblyName' was chosen instead of 'other task options' because of 'OverrideReason' informational-level warning.
the new field should be added to the XSD with brief docs

Forgind · 2021-11-15T18:20:15Z

Informational level message you mean? I think "Override" is clear enough that we shouldn't give a warning.

baronfel · 2021-11-15T18:24:10Z

Yeah, an informational level message is the correct term.

Forgind

Looks roughly good, though I'm a little worried that you changed TaskRegistry_Tests so much.

Forgind · 2021-11-15T21:56:56Z

Why this slew of test changes? It makes it look like there's a serious breaking change in here. There shouldn't need to be any test changes, right?

Ah these are holdovers from the previous solution. Will need to revert the commit that changed all these tests.

Forgind · 2021-11-15T22:03:44Z

Why this change? You can inline it instead if you want with ...out bool retrievedFromCache);

Forgind · 2021-11-15T22:05:18Z

It seems like you should be able to override an override.

Suggested change

if (overrideTask && !overriddenTasks.ContainsKey(taskName))

if (overrideTask)

Maybe a warning?

That's debatable, depending on where we want to go with this. @baronfel may lean one way or the other?

Either way, I agree a warning should be logged.

When would this happen? Some complex Condition evaluation that a user thought was exhaustive but didn't quite catch one particular corner case? It's my understanding that most <UsingTask>s are done in a block, with conditions and such to make sure that the best one is chosen. That would mean multiple Overrides applying would be unexpected.

benvillalobos · 2021-11-18T21:15:05Z

would a user ever want to know about this decision being made? if so, should there be a reason why the decision was made? e.g a OverrideReason attribute where inline doc could be added, so we could add a MS1000000: Task 'TaskName' from assembly 'AssemblyName' was chosen instead of 'other task options' because of 'OverrideReason' informational-level warning.

A message sounds reasonable when registering an overridden task. Related to another comment in this thread, should we log a warning when multiple usingtasks are marked as Override=true (with the same task name)?

baronfel · 2021-11-18T21:24:37Z

would a user ever want to know about this decision being made? if so, should there be a reason why the decision was made? e.g a OverrideReason attribute where inline doc could be added, so we could add a MS1000000: Task 'TaskName' from assembly 'AssemblyName' was chosen instead of 'other task options' because of 'OverrideReason' informational-level warning.

A message sounds reasonable when registering an overridden task. Related to another comment in this thread, should we log a warning when multiple usingtasks are marked as Override=true (with the same task name)?

I think yes, because at that point we had to make a judgement call (first seen, last seen, random based on entropy seed generated from a bee's wings) as to which Task's configuration was actually used. I think anytime we have to make a judgement call like that, the user should be told about it.

benvillalobos · 2021-11-20T00:24:02Z

I think the current test failure has to do with the build context when calling this task directly? I haven't seen an issue like this before. Will try to check for the logged warning case by building a full project that has multiple usingtasks in it, instead of calling the method directly.

Forgind

Tests look much better, thanks

rainersigwald

Can you also prepare a draft of the docs changes that will describe this? I think that'll help clarify the behavior design.

rainersigwald · 2021-11-23T20:35:08Z

The cool kids are doing it this way now (I don't actually care which you do):

Suggested change

RegisteredTaskRecord newRecord = new RegisteredTaskRecord(taskName, assemblyLoadInfo, taskFactory, taskFactoryParameters, inlineTaskRecord);

RegisteredTaskRecord newRecord = new(taskName, assemblyLoadInfo, taskFactory, taskFactoryParameters, inlineTaskRecord);

rainersigwald · 2021-11-23T20:36:58Z

You're keying off of a string name--how does that work with the partial-class-name resolution that normal task lookup does? Does this require an exact match to the invocation?

That's a wrinkle in this solution, at the moment it requires an exact match at the calling site. Another aspect to consider, how should this play with the overridetasks file?

How hard would it be to do "if the task comes from .overridetasks and there's something elsewhere with an override, error"?

Found an area that documentation will need to be updated (if we change it): https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-tasks?view=vs-2022#overridden-tasks

Tasks in these files override any other tasks with the same names, including tasks in the project file.

Looks like precedence will be:

Project UsingTask with Override attribute

Anything in .overridetasks (or .tasks?)

Project files

How hard would it be to do "if the task comes from .overridetasks and there's something elsewhere with an override, error"?

If we tried to go that route, one issue would be the lack of state in this class. When parsing a .tasks or .overridetasks file (see RegisterOverrideTasks), it calls LoadAndRegisterFromTasksFile which calls the static RegisterTasksFromUsingTaskElement. I guess we could try and determine whether we're in a .tasks or .overridetasks based on the IFileSystem that gets passed? Doesn't look like we can get the current file through it though.

rainersigwald · 2021-11-23T20:37:59Z

Why a warning and not an error?

#6783 (comment)

The conversation has mostly been about whether or not to log a warning, but could also be applied for logging an error.

IMO it's not worth failing the whole build over. My thinking is the resulting build could still be valid if the user sees this warning.

could still be valid

Or is that precisely why it's worth failing the whole build over?

My thinking is: we can always back it down to a warning if we discover a good reason to. But if we start as a warning we can't go to an error if we discover a good reason that way. So I prefer errors for new scenarios.

…not looking for an exact match, taskregistry will now return the first-defined usingtask

rainersigwald · 2021-11-29T15:47:27Z

How hard would it be to do "if the task comes from .overridetasks and there's something elsewhere with an override, error"?

… error code.

Forgind · 2021-12-14T23:16:04Z

+                string unqualifiedTaskName = taskName;
+
+                if (unqualifiedTaskName.Contains('.'))
+                {
+                    unqualifiedTaskName = taskName.Split('.').Last();
+                }


Suggested change

string unqualifiedTaskName = taskName;

if (unqualifiedTaskName.Contains('.'))

{

unqualifiedTaskName = taskName.Split('.').Last();

}

string unqualifiedTaskName = taskName.Split('.').Last();

Forgind · 2021-12-14T23:18:45Z

+                    // check every registration that exists in the list.
+                    foreach (RegisteredTaskRecord rec in recs)
+                    {
+                        // Does the same registration already exist? (same exact name)


Please only include useful comments. This and the past two comments added nothing to the line of code immediately below them. Comments are easily forgotten when changing code.

Forgind · 2021-12-14T23:26:21Z

+                    // Create a dictionary containing the unqualified name (for quick lookups when the task is called).
+                    // Place the new record using the potentially fully qualified name to account for partial matches.
+                    overriddenTasks.Add(unqualifiedTaskName, new List<RegisteredTaskRecord>());
+                    overriddenTasks[unqualifiedTaskName].Add(newRecord);


Suggested change

// Create a dictionary containing the unqualified name (for quick lookups when the task is called).

// Place the new record using the potentially fully qualified name to account for partial matches.

overriddenTasks.Add(unqualifiedTaskName, new List<RegisteredTaskRecord>());

overriddenTasks[unqualifiedTaskName].Add(newRecord);

// New record's name may be fully qualified. Use it anyway to account for partial matches.

List<RegisteredTaskRecord> unqualifiedTaskNameMatches = new();

unqualifiedTaskNameMatches.Add(newRecord);

overriddenTasks.Add(unqualifiedTaskName, unqualifiedTaskNameMatches);

?

Saves a dictionary access, too.

benvillalobos · 2022-02-03T22:39:23Z

/azp run

azure-pipelines · 2022-02-03T22:39:33Z

Azure Pipelines successfully started running 1 pipeline(s).

rainersigwald · 2022-02-04T13:04:15Z

Prioritization order of usingtasks:

Project-level usingtask override

.tasks/.overridetasks file

UsingTask defined elsewhere

Is this right? Shouldn't it be

Project-level UsingTask override
.overridetasks file
Project-level UsingTask
.tasks file

?

rainersigwald

Looking great, just a couple of things to look at!

benvillalobos · 2022-02-14T17:08:46Z

/azp run

azure-pipelines · 2022-02-14T17:08:57Z

Azure Pipelines successfully started running 1 pipeline(s).

Forgind · 2022-02-16T17:11:14Z

@benvillalobos, resolved a merge conflict in case you want to look at it, but it was pretty straightforward.

benvillalobos mentioned this pull request Aug 23, 2021

UsingTask overrides do not apply if newly specifying Architecture/Runtime #5541

Closed

benvillalobos marked this pull request as draft August 23, 2021 23:28

benvillalobos changed the title ~~Return First-Defined UsingTask~~ Return First-Defined UsingTask With Architecture Metadata Sep 1, 2021

Forgind reviewed Nov 8, 2021

View reviewed changes

Comment thread src/Build/Instance/TaskRegistry.cs Outdated

benvillalobos commented Nov 8, 2021

View reviewed changes

Comment thread src/Build/Instance/TaskRegistry.cs Outdated

benvillalobos marked this pull request as ready for review November 8, 2021 22:57

benvillalobos commented Nov 8, 2021

View reviewed changes

Forgind requested a review from baronfel November 15, 2021 16:36

baronfel reviewed Nov 15, 2021

View reviewed changes

Forgind approved these changes Nov 15, 2021

View reviewed changes

benvillalobos changed the title ~~Return First-Defined UsingTask With Architecture Metadata~~ Allow an Override Flag in UsingTasks Nov 18, 2021

Forgind approved these changes Nov 22, 2021

View reviewed changes

Comment thread src/Build/Instance/TaskRegistry.cs Outdated

Forgind reviewed Nov 22, 2021

View reviewed changes

Comment thread src/Build/Resources/Strings.resx Outdated

Forgind reviewed Nov 22, 2021

View reviewed changes

Comment thread src/Build/Instance/TaskRegistry.cs Outdated

rainersigwald reviewed Nov 23, 2021

View reviewed changes

Code cleanup, only TryGetValue when looking for an exact match. When …

d464a0e

…not looking for an exact match, taskregistry will now return the first-defined usingtask

benvillalobos force-pushed the taskregistry-firstdefined branch from f918fa2 to 91cc04a Compare November 23, 2021 22:58

benvillalobos added 2 commits November 24, 2021 11:12

Add Override & OverrideLocation to the public API

aa11c51

Log an error instead of a warning when duplicate overrides are created

fbcbc98

benvillalobos force-pushed the taskregistry-firstdefined branch from 91cc04a to fbcbc98 Compare November 24, 2021 19:17

rainersigwald reviewed Nov 29, 2021

View reviewed changes

benvillalobos added 5 commits December 6, 2021 15:47

Remove ref file

a8aa234

Remove netstandard ref file

c167c0b

Remove comment logging when picking an overridden task. Decrement msb…

2b6fbb0

… error code.

Account for decremented error code in test.

e3e4fd7

Finalize task lookup logic

304d0bb

benvillalobos added the review-by-diff label Dec 9, 2021

Forgind reviewed Dec 14, 2021

View reviewed changes

benvillalobos added 4 commits January 11, 2022 16:03

PR Feedback. Code & comment cleanup

4976a81

Fix CA1825

60c65ab

Merge upstream/main

2134e29

Fix shipped API for analyzers

6c860e9

rainersigwald reviewed Feb 4, 2022

View reviewed changes

Comment thread src/Build.UnitTests/BackEnd/TaskRegistry_Tests.cs

Comment thread src/Build/Instance/TaskRegistry.cs Outdated

Comment thread src/Build/Instance/TaskRegistry.cs Outdated

Comment thread src/Build/Resources/Strings.resx

Comment thread src/Build/Resources/Strings.resx Outdated

benvillalobos added 2 commits February 10, 2022 15:28

Log where the override usingtask was created

f6f96da

Add resource update

5d9c246

rainersigwald approved these changes Feb 14, 2022

View reviewed changes

benvillalobos mentioned this pull request Feb 14, 2022

Windows On Full Framework Flaky Tests #7391

Closed

Merge branch 'main' into taskregistry-firstdefined

7058577

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 16, 2022

rokonec merged commit 047227b into dotnet:main Feb 23, 2022

	if (overrideTask && !overriddenTasks.ContainsKey(taskName))
	if (overrideTask)

	RegisteredTaskRecord newRecord = new RegisteredTaskRecord(taskName, assemblyLoadInfo, taskFactory, taskFactoryParameters, inlineTaskRecord);
	RegisteredTaskRecord newRecord = new(taskName, assemblyLoadInfo, taskFactory, taskFactoryParameters, inlineTaskRecord);

Conversation

benvillalobos commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Notes

Expected behaviors

If multiple overrides override a task with the exact same name (fully qualified), MSB4275 will be logged

Prioritization order of usingtasks:

Uh oh!

benvillalobos commented Aug 23, 2021

Uh oh!

rainersigwald commented Aug 24, 2021

Uh oh!

benvillalobos commented Aug 31, 2021

Uh oh!

benvillalobos commented Aug 31, 2021

Uh oh!

benvillalobos commented Sep 1, 2021

Uh oh!

benvillalobos commented Sep 1, 2021

Uh oh!

benvillalobos commented Sep 1, 2021

Uh oh!

benvillalobos commented Sep 2, 2021

Uh oh!

benvillalobos commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baronfel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind commented Nov 15, 2021

Uh oh!

baronfel commented Nov 15, 2021

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benvillalobos commented Nov 18, 2021

Uh oh!

baronfel commented Nov 18, 2021

Uh oh!

benvillalobos commented Nov 20, 2021

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benvillalobos commented Aug 23, 2021 •

edited

Loading

benvillalobos commented Nov 2, 2021 •

edited

Loading

baronfel left a comment •

edited

Loading