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 /graph:noBuild #6016

Merged
merged 5 commits into from Feb 8, 2021
Merged

Add /graph:noBuild #6016

merged 5 commits into from Feb 8, 2021

Conversation

@cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Jan 10, 2021

This new option constructs the graph but does not build the nodes.

This is useful for a few use cases:

  • determine if a large repo can evaluate all of its projects (e.g., is VS setup right, are all imports magically pointing to where they should, etc)
  • easily investigate evaluation perf. With noBuild, the only thing that MSBuild does is evaluate all projects under a single process, so it's easier to throw it under a profiler.
  • in the future when project caching is available, it can be used to warm up / download the caches but not do the build.
  • generally makes life easier for people that only want to investigate evaluation and not the build. The binlog also shows up nicely containing all the evaluation nodes.
@cdmihai cdmihai force-pushed the cdmihai:noBuild branch from 4f47ed2 to 3353e49 Jan 10, 2021
@cdmihai
Copy link
Contributor Author

@cdmihai cdmihai commented Jan 10, 2021

@rainersigwald I added a record type with init properties and it seems the CI is using an older compiler that does not recognize these. Is it fairly easy to get CI updated, or should I replace the records with manual equals / hashcode implementations? 😢

@rainersigwald
Copy link
Contributor

@rainersigwald rainersigwald commented Jan 11, 2021

I bet the update to Arcade 5/.NET 5 SDK will handle that. @BenVillalobos is pretty close on that so I wouldn't rewrite.

Copy link
Contributor

@rainersigwald rainersigwald left a comment

LGTM -- should we fork for 16.9 so we have a place to land this?

src/MSBuild/Resources/Strings.resx Show resolved Hide resolved
src/Shared/AssemblyUtilities.cs Outdated Show resolved Hide resolved
@rainersigwald
Copy link
Contributor

@rainersigwald rainersigwald commented Jan 11, 2021

/azp run

@azure-pipelines
Copy link

@azure-pipelines azure-pipelines bot commented Jan 11, 2021

Azure Pipelines successfully started running 1 pipeline(s).

while (blockedNodes.Count > 0 || buildingNodes.Count > 0)
{
waitHandle.WaitOne();

This comment has been minimized.

@Forgind

Forgind Jan 14, 2021
Member

I'm a little confused here. It looks like the waitHandle receives signals below, but how would it ever get there if the thread is paused indefinitely here?

This comment has been minimized.

@cdmihai

cdmihai Jan 14, 2021
Author Contributor

This just an extracted method, not new logic, you can skip it if you wish. To answer your question, whenever there's potential new work to do the waitHandle gets signaled. Check out all the places in the method that call Set() on that waitHandle.

@cdmihai cdmihai force-pushed the cdmihai:noBuild branch 2 times, most recently from 317f2d5 to 25ba2ed Jan 16, 2021
@cdmihai
Copy link
Contributor Author

@cdmihai cdmihai commented Jan 20, 2021

@radical, do you know how to update the compilers in the Mono CI to a new enough version to parse C# record types? CI is currently failing because of that: https://dev.azure.com/dnceng/public/_build/results?buildId=959029&view=logs&j=09f5a607-bbad-5164-6b70-f20ebe806390&t=64b2bbfe-7647-5e20-2e5f-fab3af842098

Copy link
Member

@Forgind Forgind left a comment

LGTM!

@@ -7,6 +7,14 @@

namespace Microsoft.Build.Graph
{
public record GraphBuildOptions

This comment has been minimized.

@Forgind

Forgind Jan 27, 2021
Member

Why make this a record if it only has one thing in it? It seems like it's making it unnecessarily complicated, since then in addition to checking whether it's null, you need to check whether .Build is true or false.

This comment has been minimized.

@cdmihai

cdmihai Jan 27, 2021
Author Contributor

Because there will be more things in it in the future and since MSBuild APIs are set in stone until the end of time I'd rather start with a 1 member record than successive API changes that progressively add yet another public constructor with yet another parameter followed eventually by an actual struct. Basically avoiding the fiasco with the Project constructors which got eventually followed by ProjectOptions.

using var cacheService = cacheServiceTask.Result;
if (submission.BuildRequestData.GraphBuildOptions.Build)
{
var cacheServiceTask = Task.Run(() => SearchAndInitializeProjectCachePluginFromGraph(projectGraph));

This comment has been minimized.

@Forgind

Forgind Jan 27, 2021
Member

Just out of curiosity—why did you reorder these? Shouldn't matter either way.

This comment has been minimized.

@cdmihai

cdmihai Jan 27, 2021
Author Contributor

Pedantism. Put the task which I think takes the longest first. Thread can loose context right before issuing the second task so might as well have the longest running one first.

/// <summary>
/// If false, the graph is constructed but the nodes are not built.
/// </summary>
public bool Build { get; init; } = true;

This comment has been minimized.

@Forgind

Forgind Jan 27, 2021
Member

I wasn't familiar with this, and it looks like it's too new for mono. Might change it to set { init; return whatever; }

This comment has been minimized.

@cdmihai

cdmihai Jan 27, 2021
Author Contributor

Mono will get eventually updated and until then we'll just disable it. See #6059

Copy link
Member

@BenVillalobos BenVillalobos left a comment

Mostly LGTM! Pending build errors & questions

@@ -1233,7 +1233,7 @@ string outputResultsCache

if (!restoreOnly)
{
if (graphBuild)
if (graphBuildOptions != null)

This comment has been minimized.

@BenVillalobos

BenVillalobos Feb 3, 2021
Member

&& graphBuildOptions.Build?

This comment has been minimized.

@cdmihai

cdmihai Feb 3, 2021
Author Contributor

graphBuildOptions.Build gets acted upon later on in the BuildManager, after the graph is constructed. That's the whole point, construct the graph but not build it.


if (parameter.Trim().Equals("NoBuild", StringComparison.OrdinalIgnoreCase))
{
options = options with {Build = false};

This comment has been minimized.

@BenVillalobos

BenVillalobos Feb 3, 2021
Member

Should this data structure be a record if we actually want to be able to set the value?

This comment has been minimized.

@cdmihai

cdmihai Feb 3, 2021
Author Contributor

Why should it not be a record?

[Fact]
public void GraphBuildShouldBeAbleToConstructGraphButSkipBuild()
{
var graph = Helpers.CreateProjectGraph(_env, new Dictionary<int, int[]> {{1, new[] {2, 3}}});

This comment has been minimized.

@BenVillalobos

BenVillalobos Feb 3, 2021
Member

Nit: list out parameter names to make it easier to read. You did it just below this.

This comment has been minimized.

@BenVillalobos

BenVillalobos Feb 3, 2021
Member

Random question: Any worry about this method being passed invalid data?

This comment has been minimized.

@cdmihai

cdmihai Feb 3, 2021
Author Contributor

It's a helper method for tests, if it's passed invalid data some test somewhere will fail.


IReadOnlyDictionary<ProjectGraphNode, ImmutableList<string>> targetLists = targetListTask.Result;
using var cacheService = cacheServiceTask.Result;
if (submission.BuildRequestData.GraphBuildOptions.Build)

This comment has been minimized.

@BenVillalobos

BenVillalobos Feb 3, 2021
Member

Do we need a null check here?

This comment has been minimized.

@cdmihai

cdmihai Feb 3, 2021
Author Contributor

The GraphBuildRequestData constructors ensures it's never null.

@Forgind
Copy link
Member

@Forgind Forgind commented Feb 3, 2021

And the build error is because the mono build doesn't have an updated version of roslyn. Resolved by #6059 (admittedly not ideally—by updating roslyn for mono—but by disabling it).

@cdmihai cdmihai force-pushed the cdmihai:noBuild branch from 25ba2ed to aaea331 Feb 3, 2021
@cdmihai
Copy link
Contributor Author

@cdmihai cdmihai commented Feb 4, 2021

/azp run

@azure-pipelines
Copy link

@azure-pipelines azure-pipelines bot commented Feb 4, 2021

Azure Pipelines successfully started running 1 pipeline(s).
@cdmihai cdmihai force-pushed the cdmihai:noBuild branch from aaea331 to 3942a2c Feb 4, 2021
@Forgind Forgind merged commit 3eb41d4 into dotnet:master Feb 8, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210203.19 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
@cdmihai cdmihai deleted the cdmihai:noBuild branch Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants