Skip to content

Backward-compatibility with KeyValuePair<string, string> metadata items [#8864]#8870

Merged
JanKrivanek merged 3 commits intodotnet:mainfrom
jehhynes:main
Jun 29, 2023
Merged

Backward-compatibility with KeyValuePair<string, string> metadata items [#8864]#8870
JanKrivanek merged 3 commits intodotnet:mainfrom
jehhynes:main

Conversation

@jehhynes
Copy link
Copy Markdown
Contributor

@jehhynes jehhynes commented Jun 12, 2023 •
edited
Loading

Fixes #8864

Context

TaskExecutionHost.cs

Changes Made

Fallback to the prior behavior of expecting metadata items of type KeyValuePair<string, string>

Testing

This has not been tested

@jehhynes
Copy link
Copy Markdown
Contributor Author

jehhynes commented Jun 12, 2023

@dotnet-policy-service agree

rainersigwald
rainersigwald reviewed Jun 12, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I understand the problem, I'm not sure this fix is sufficient; there could theoretically be an ITaskItem implementation that returned a totally custom type with non-KVP non-DictionaryEntry backing for its IEnumerable implementation.

Taking this would certainly cover the likely implementations, but I'm starting to think we should go back to the foreach for this fallback code path.

@rainersigwald
Copy link
Copy Markdown
Member

rainersigwald commented Jun 12, 2023

(oh and also . . . thanks for the PR!)

@rainersigwald rainersigwald added this to the VS 17.7 milestone Jun 12, 2023
@AR-May AR-May requested a review from donJoseLuis June 13, 2023 13:35
donJoseLuis
donJoseLuis approved these changes Jun 13, 2023
View reviewed changes
Copy link
Copy Markdown

@donJoseLuis donJoseLuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few minor comments.

@jehhynes jehhynes force-pushed the main branch 2 times, most recently from 7b58e87 to a4e179d Compare June 13, 2023 18:23
@jehhynes
Copy link
Copy Markdown
Contributor Author

jehhynes commented Jun 13, 2023 •
edited
Loading

@donJoseLuis @rainersigwald I have updated the PR based on the suggestions to use foreach, InternalErrorException, and if statements.
I believe I will let you guys take it from here - as far as unit testing and discussiong @donJoseLuis 's other concerns. Thanks!

P.S. Do we care that we are assuming all DictionaryEntry's will be <string, string>?

rainersigwald
rainersigwald requested changes Jun 13, 2023
View reviewed changes
@rainersigwald
Copy link
Copy Markdown
Member

rainersigwald commented Jun 13, 2023

P.S. Do we care that we are assuming all DictionaryEntry's will be <string, string>?

No, that's ok--it's the only thing that would ever have worked AFAIK.

@jehhynes
Copy link
Copy Markdown
Contributor Author

jehhynes commented Jun 13, 2023

Updated to loop DictionaryEntry's in the foreach.

JanKrivanek
JanKrivanek requested changes Jun 14, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for diving into this!

If the call to SetMetadata is intentionall - please explain the reasoning in the description.

Without that - it looks good to go

JanKrivanek
JanKrivanek approved these changes Jun 14, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

rainersigwald
rainersigwald reviewed Jun 14, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more small nit (we can avoid reallocations by specifying the size of the list up front). Thanks for working through this, it's looking great!

@JaynieBai, could you please write a test for this case? We'll need a task that returns a custom ITaskItem implementation that has a custom IDictionary type returned from CloneCustomMetadata().

@jehhynes @rainersigwald
Update src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs
91f6186 
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
@JaynieBai
Copy link
Copy Markdown
Member

JaynieBai commented Jun 16, 2023

One more small nit (we can avoid reallocations by specifying the size of the list up front). Thanks for working through this, it's looking great!

@JaynieBai, could you please write a test for this case? We'll need a task that returns a custom ITaskItem implementation that has a custom IDictionary type returned from CloneCustomMetadata().

Yeah, I will do that

@JanKrivanek JanKrivanek mentioned this pull request Jun 22, 2023
Closed
JaynieBai added a commit that referenced this pull request Jun 27, 2023
@JaynieBai
Add unit test for #8870
2ad1376
@JaynieBai JaynieBai mentioned this pull request Jun 28, 2023
Merged
@JanKrivanek
Copy link
Copy Markdown
Member

JanKrivanek commented Jun 29, 2023

@rainersigwald - all expressed concerns seem to be addressed - do you want to revise?

@JanKrivanek JanKrivanek merged commit c6277a4 into dotnet:main Jun 29, 2023
@JanKrivanek
Copy link
Copy Markdown
Member

JanKrivanek commented Jun 29, 2023

/backport to vs17.7

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 29, 2023

Started backporting to vs17.7: https://github.com/dotnet/msbuild/actions/runs/5415050762

@github-actions github-actions bot mentioned this pull request Jun 29, 2023
Merged
rainersigwald pushed a commit that referenced this pull request Jul 12, 2023
@rainersigwald
Backward-compatibility with KeyValuePair<string, string> metadata ite…
8ebc5ca 
…ms (#8870)

Backport #8870 to vs17.7.
JaynieBai added a commit that referenced this pull request Jul 28, 2023
@JaynieBai
Add unit test for #8870 (#8961)
6c1bb22 
Fixes 8939

Context
#8870 (comment)

Changes Made
Add a task that returns a custom ITaskItem implementation that has a custom IDictionary type returned from CloneCustomMetadata()
Add unit test TestTaskDictionaryOutputItems()
dotnet-bot pushed a commit to dotnet/dotnet that referenced this pull request Jul 31, 2023
@dotnet-maestro
[msbuild] Sync 24ee0c7 → e2e438e
07d60a2 
Diff: https://github.com/dotnet/msbuild/compare/24ee0c75a7c5a9924c68157ae7f8e0ce6c9adca3..e2e438ef48eec8cf83d81cfc66cf3bb363ff5513

From: dotnet/msbuild@24ee0c7
To: dotnet/msbuild@e2e438e

Commits:
  - [main] Update dependencies from dotnet/arcade (#9059)
    dotnet/msbuild@881abda
  - Merge pull request #9066 from JanKrivanek/proto/tl-dotnettests-disable
    dotnet/msbuild@fd4d0e5
  - Add unit test for dotnet/msbuild#8870 (#8961)
    dotnet/msbuild@6c1bb22
  - fixes dotnet/msbuild#8958  TerminalLogger in .NET 8.0.100-preview.6 issues audible alerts on iTerm2 (#9060)
    dotnet/msbuild@ce59c70
  - TaskFactoryWrapper: guard against multi-threaded access to dictionaries (#8928)
    dotnet/msbuild@edeacbb
  - Resource for invalid -tl value (#9078)
    dotnet/msbuild@e2e438e

[[ commit created by automation ]]
This was referenced Jul 29, 2025
Closed
Closed
Closed
@dependabot dependabot bot mentioned this pull request Aug 6, 2025
Closed
@dependabot dependabot bot mentioned this pull request Aug 15, 2025
Closed
This was referenced Oct 15, 2025
Open
Closed
Closed
Closed
Closed
This was referenced Nov 13, 2025
Closed
Open
Open
Closed
Closed
Open
This was referenced Nov 26, 2025
Closed
Closed
Merged
This was referenced Dec 24, 2025
Merged
Merged
@dependabot dependabot bot mentioned this pull request Mar 6, 2026
Merged
This was referenced Mar 27, 2026
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JanKrivanek JanKrivanek JanKrivanek approved these changes

@rainersigwald rainersigwald rainersigwald approved these changes

+2 more reviewers

@JaynieBai JaynieBai JaynieBai left review comments

@donJoseLuis donJoseLuis donJoseLuis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

VS 17.7

Development

Successfully merging this pull request may close these issues.

[Bug]: MSB4028 "XmlQuery" task's outputs could not be retrieved from the "Values" parameter. Specified cast is not valid.

5 participants

@jehhynes @rainersigwald @JaynieBai @JanKrivanek @donJoseLuis