-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add task for metadata resolution #9313
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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!
82b80c1
to
84cc9c6
Compare
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this 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.
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 . |
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. |
@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? |
Yes please. Leading |
ae55275
to
0eb79f5
Compare
There was a problem hiding this 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.
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. |
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.