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

Add task for metadata resolution #9313

Conversation

YuliiaKovalova
Copy link
Member

Context

The metadata is needed by VS. Since DTB call to msbuild from the legacy project system is already asynchronous, this change is the optimal solution for requesting assembly info.

Changes Made

Add GetComAssembliesMetadata task for gathering assembly metadata.

Testing

UTs are added.

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

I see you're making changes so I'm flushing my pending comments and will continue with the latest version.

src/Tasks/Microsoft.Common.tasks Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.overridetasks Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Build.Tasks.csproj Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova marked this pull request as draft October 9, 2023 14:58
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

I've left a few more comments inline.

What makes the new task useful only for COM references? The name GetComAssembliesMetadata suggests that we're dealing with something COM-related but it appears to be reading a bunch of general .NET metadata, so it's not clear if the name is justified. Thank you!

src/Tasks/AssemblyDependency/AssemblyAttributes.cs Outdated Show resolved Hide resolved
src/Tasks/AssemblyDependency/AssemblyInformation.cs Outdated Show resolved Hide resolved
src/Tasks/AssemblyDependency/AssemblyInformation.cs Outdated Show resolved Hide resolved
src/Tasks/GetComAssembliesMetadata.cs Outdated Show resolved Hide resolved
src/Tasks/GetComAssembliesMetadata.cs Outdated Show resolved Hide resolved
src/Tasks/GetComAssembliesMetadata.cs Outdated Show resolved Hide resolved
src/Tasks/AssemblyDependency/AssemblyAttributes.cs Outdated Show resolved Hide resolved
src/Tasks/AssemblyDependency/AssemblyInformation.cs Outdated Show resolved Hide resolved
src/Tasks.UnitTests/GetComAssembliesMetadata_Tests.cs Outdated Show resolved Hide resolved
src/Tasks.UnitTests/GetComAssembliesMetadata_Tests.cs Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/add_task_for_metadata_resolution branch from 82b80c1 to 84cc9c6 Compare October 11, 2023 08:08
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.

Just a quick review - I'll need to have deeper look

src/Tasks/AssemblyDependency/AssemblyAttributes.cs Outdated Show resolved Hide resolved
src/Tasks/GetAssembliesMetadata.cs Outdated Show resolved Hide resolved
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Looks great, I've left a few mostly minor comments.

src/Tasks.UnitTests/GetAssembliesMetadata_Tests.cs Outdated Show resolved Hide resolved
src/Tasks/GetAssembliesMetadata.cs Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova marked this pull request as ready for review October 13, 2023 08:38
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

It's unfortunate that we have to expand the public surface of MSBuild for a VS-only feature. I guess there's no easy way around it so this is acceptable. Reviewers, please chime in.

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
@rainersigwald
Copy link
Member

(Context: CSProj is currently doing some assembly metadata scanning on the main VS thread. This work is addressing the issue by adding a design-time target+task as a replacement.)

Can y'all help me understand why "move that to a thread" isn't a good option here?

@YuliiaKovalova
Copy link
Member Author

(Context: CSProj is currently doing some assembly metadata scanning on the main VS thread. This work is addressing the issue by adding a design-time target+task as a replacement.)

Can y'all help me understand why "move that to a thread" isn't a good option here?

It's complex and error-prone due to COM objects usage, compared to the idea of getting the needed information during already asynchronous DTB .

@ladipro
Copy link
Member

ladipro commented Oct 13, 2023

Adding to Yuliia's comment, keeping it in VS would be non-trivial threading-wise, especially since it is happening deep in the C++ implementation of the legacy project system. We feel it fits better with DT builds which CSProj is already doing - this is just one target to add to the build request. We get caching of the result for free, for example.

@YuliiaKovalova
Copy link
Member Author

@rainersigwald, could you help me to understand naming convention here for props and target? Since it is planned to use it internally only, do I need to use '_' at the beginning of the names?

@rainersigwald
Copy link
Member

Since it is planned to use it internally only, do I need to use '_' at the beginning of the names.

Yes please. Leading _ is the closest we have to a "private" modifier.

@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/add_task_for_metadata_resolution branch from ae55275 to 0eb79f5 Compare October 16, 2023 08:12
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.

Overall looks ok, but I have a bunch of questions and comments.

src/Tasks/Microsoft.Common.overridetasks Show resolved Hide resolved
src/Tasks/GetAssembliesMetadata.cs Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved
src/Tasks/Microsoft.Common.tasks Outdated Show resolved Hide resolved
@drewnoakes
Copy link
Member

Do we understand what it would take to have CSProj ship and use its own design time targets that would only be loaded within VS design time builds? This target seems like a great candidate for that. There was a similar question recently that could have also used such targets.

@YuliiaKovalova
Copy link
Member Author

Do we understand what it would take to have CSProj ship and use its own design time targets that would only be loaded within VS design time builds? This target seems like a great candidate for that. There was a similar question recently that could have also used such targets.

Drew, we have discussed in internally, unfortunately it's a huge chunk of work that potentially affects VS performance due to adding new assemblies in the process.

@YuliiaKovalova YuliiaKovalova merged commit c8b80b9 into dotnet:main Nov 15, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants