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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jun 29, 2023
Merged

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

merged 3 commits into from
Jun 29, 2023
+9 −3

Conversation

jehhynes
Copy link
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
Contributor Author

@dotnet-policy-service agree

rainersigwald
rainersigwald reviewed Jun 12, 2023
View reviewed changes
Copy link
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
Member

(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

@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
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
Copy link
Member

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
Contributor Author

Updated to loop DictionaryEntry's in the foreach.

JanKrivanek
JanKrivanek requested changes Jun 14, 2023
View reviewed changes
Copy link
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

@jehhynes
Backward-compatibility with KeyValuePair<string, string> metadata ite…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
7f6b2a9 
…ms [dotnet#8864]
JanKrivanek
JanKrivanek approved these changes Jun 14, 2023
View reviewed changes
Copy link
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
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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
91f6186 
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
@JaynieBai
Copy link
Member

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

@rainersigwald
Avoid allocations (and fix SA1509 nitpick)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
3298b19
@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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
2ad1376
@JaynieBai JaynieBai mentioned this pull request Jun 28, 2023
Merged
@JanKrivanek
Copy link
Member

@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
Member

/backport to vs17.7

@github-actions
Copy link
Contributor

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
@jehhynes @rainersigwald
Backward-compatibility with KeyValuePair<string, string> metadata ite…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
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)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
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 ]]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JanKrivanek JanKrivanek

@JaynieBai JaynieBai

@rainersigwald rainersigwald

@donJoseLuis donJoseLuis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
VS 17.7
5 participants
@jehhynes @rainersigwald @JaynieBai @JanKrivanek @donJoseLuis