-
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
Exp/serializable projectevaluation #6262
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.
Looks good! I've left a nit and a question inline.
/// Creates a ProjectInstance from an external created <see cref="Project"/>. | ||
/// Properties and items are cloned immediately and only the instance data is stored. | ||
/// </summary> | ||
public ProjectInstance(Project project, ProjectInstanceSettings settings) |
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.
How is this different from the other ProjectInstance(Evaluation.Project.Data) constructor which is called from Project.CreateProjectInstance? They seem the same to me, so I'd rather not add this additional constructor (unless I'm missing something).
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.
The intent of this new public constructor is that it /// Creates a ProjectInstance from an external created <see cref="Project"/>.
Data
and the Data
based constructor are both internal. Going that route would mean exposing the internal implementation detail that is Data
and making it a linkable object that the client has to construct their external Project around and pass that in for this purpose.
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.
Ah, okay, let me rephrase it to ensure that I got it right: your scenario is to create a ProjectInstance
object residing in Process_1 based on a Project
object residing in Process_2. You want to avoid marshaling the Project.Data
object from Process_2 to Process_1 and instead have the ProjectInstance
constructor from Process_1 do remote calls for each Project
data field (one "rpc" call for properties, one for items, one for targets, etc).
Yea, in this case reading the data directly from the remote Project object is more straighforward than manually coding up a stub/proxy for Project.Data
as well. Assuming the two processes always reside on the same machine, otherwise the rpc overhead might make it worth it to serialize the entire Project.Data over one rpc call.
However, I'd just delete the old ProjectInstance(Evaluation.Project.Data)
constructor. It's just code duplication with your new constructor.
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.
Yes, that's a good summary. The old ProjectInstance(Evaluation.Project.Data)
constructor still will get used as-is in the non cached mode in VS where MSBuild gets called for actual evaluation. I would like to keep it that way for feature isolation reasons, atleast until the cache mode feature which will initially go in behind a feature flag gets more test coverage.
Fixes #6260
Context
The evaluation result is being cached now on the CPS side to speed up solution load. For the cases where CPS has it's linked read-only Project, we would like to create a ProjectInstance as well from it, instead of triggering a evaluation for the sake of creating a ProjectInstance
Changes Made
Testing
Added a test for the new constructor.
Notes