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 ProjectOptions.Interactive so that callers can specify that project evaluation can be interactive #8533

Merged
merged 6 commits into from
Mar 7, 2023

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Mar 5, 2023

Fixes #8523

Context

When a build happens the BuildParameters object passes along the Interactive property when creating ProjectInstance objects. During evaluation, the Evaluator then sets the built-in MSBuildInteractive property to true and the SDK resolver context is also set.

However, when you create an instance of a ProjectInstance object via the ProjectInstance.FromFile(string, ProjectOptions) method, there is no way to indicate that the evaluation should be interactive.

Changes Made

This change adds a new Interactive property to the ProjectOptions object. The value is passed to the evaluator and the MSBuildInteractive MSBuild property is now set.

I implemented this for:

  • ProjectInstance.FromFile(string, ProjectOptions)
  • ProjectInstance.FromProjectRootElement(ProjectRootElement, ProjectOptions)
  • Project.FromFile(string, ProjectOptions)
  • Project.FromProjectRootElement(ProjectRootElement, ProjectOptions)
  • Project.FromXmlReader(XmlReader, ProjectOptions)

Testing

I added unit tests for all affected methods by testing what happens when Interactive is set to true or false. When the value is true, it is expected that the MSBuildInteractive MSBuild property is set, otherwise the property value should be an empty string.

Notes

  • I broke up the commits separately in case we don't want to touch Project (although it was trivial to implement).
  • The last commit migrates ProjectInstance_Internal_Tests to Shouldly, commit ff9398d46008b1e2389dafb083cf6b86fb40eedb has a simpler diff showing the added tests.

@@ -3733,7 +3741,8 @@ private void ReevaluateIfNecessary(ILoggingService loggingServiceForEvaluation,
s_buildEventContext,
evaluationContext.SdkResolverService,
BuildEventContext.InvalidSubmissionId,
evaluationContext);
evaluationContext,
_interactive);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its difficult to tell in this diff but this is where the interactive flag is passed to the evaluator.

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

I support doing Project too.

src/Build.UnitTests/Definition/Project_Internal_Tests.cs Outdated Show resolved Hide resolved
@jeffkl jeffkl force-pushed the projectoptions-interactive branch from 40c8da7 to 9eefbc5 Compare March 6, 2023 17:50
@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Mar 6, 2023
@JaynieBai JaynieBai merged commit 747ecc3 into dotnet:main Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ProjectInstance should allow a way to specify Interactive
4 participants