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

Exp/serializable projectevaluation #6262

Merged
merged 5 commits into from Mar 23, 2021
Merged

Conversation

jackcartman
Copy link
Contributor

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

  • Add a constructor to ProjectInstance that builds itself from the Project.
  • Also exposed the existing GetAllGlobs override to ProjectLink. Previously never exposed since ProjectLink was used only for csproj that does not really care about globs.

Testing

Added a test for the new constructor.

Notes

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 good! I've left a nit and a question inline.

src/Build/Instance/ProjectInstance.cs Outdated Show resolved Hide resolved
src/Build/Instance/ProjectInstance.cs Show resolved Hide resolved
/// 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)
Copy link
Contributor

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).

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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.

@Forgind Forgind merged commit a4e26e4 into main Mar 23, 2021
@rainersigwald rainersigwald deleted the exp/serializable-projectevaluation branch April 15, 2021 14:09
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.

Serializable MSBuild.Project support
5 participants