Implementing tracking of environment variables #4437
Conversation
…perties) for graph evaluation.
|
|
src/Build/Graph/ProjectGraph.cs
Outdated
| /// <summary> | ||
| /// Gets a distinct enumeration of all of the environment variables reads during evaluation. | ||
| /// </summary> | ||
| public IEnumerable<string> EnvironmentVariableReads => _projectNodesTopologicallySorted?.Value.Select(p => p.ProjectInstance).SelectMany(pi => pi.EnvironmentVariableReads).Distinct(); |
There was a problem hiding this comment.
EnvironmentVariableReads [](start = 154, length = 24)
This may be premature optimization, so feel free to ignore my comment, but it occurs to me that there is going to be a relatively big set of common env variables that are the same across every project (platform, configuration, etc).
So by having each project carry its own set, and aggregating that at the project graph level could increase memory footprint unnecessary. Each project could report reads an uninitialize vars to the project graph as they are being discovered, so each item will be there only once, and they can be added to a set or similar so de deduplication will also be unnecessary.
There was a problem hiding this comment.
I've changed this aggregation to perform better (not leaving it to LINQ operations) in more recent commits. Others have suggested using an event mechanism, but as I responded below, I'm not sure how to nicely plumb such a system into the current architecture where it would have pass through ProjectGraph --> GraphBuilder --> ProjectInstance.
| /// Sets a property which does not come from the Xml. | ||
| /// </summary> | ||
| P SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved); | ||
| P SetProperty(string name, string evaluatedValueEscaped, bool isGlobalProperty, bool mayBeReserved, bool isEnvVar = false); |
There was a problem hiding this comment.
My suggestion is to add environment variable reads and uninitialized property reads to Project as well, not just ProjectInstance.
In certain cases, ProjectInstances are created from Project instances via https://github.com/microsoft/msbuild/blob/master/src/Build/Definition/Project.cs#L2696. When this happens, the evaluation that took places against a Project.Data based IEvaluatorData gets to initialize the ProjectInstance.
Quickbuild and VS do this. @dfederm, would qb use this information ?
There was a problem hiding this comment.
QuickBuild mostly just uses ProjectInstance. We use a few small pieces of Project when we need unevaluated values, but only when necessary. We prefer using ProjectInstance when possible.
There was a problem hiding this comment.
Do you initialize the ProjectInstance from a Project?
src/Build/Graph/ProjectGraph.cs
Outdated
| /// <summary> | ||
| /// Get a distinct enumeration of all the uninitialized property reads during evaluation. | ||
| /// </summary> | ||
| public IEnumerable<string> UninitializedPropertyReads => _projectNodesTopologicallySorted?.Value.Select(p => p.ProjectInstance).SelectMany(pi => pi.UninitializedPropertyReads).Distinct(); |
There was a problem hiding this comment.
UninitializedPropertyReads [](start = 35, length = 26)
At least at the static api level, I would rename this to something like 'AbsentEnvironmentVariableProbes'.
Uninitialized properties, even though represent the same concept, involves a couple of hops in the semantic that MSBuild gives to property/env var interactions, and looks somewhat asymmetrical to 'EnvironmentVariableReads'
| /// Tracks this property read if it's from an environment variable or uninitialized. | ||
| /// </summary> | ||
| /// <param name="name">The name of the property being read.</param> | ||
| private void TrackPropertyRead(string name, bool propertyMissing) |
There was a problem hiding this comment.
I think issuing an event for each of these would be better than storing them in collections:
- There is a lot of duplication in this data, most evaluations will have the same environment reads. This repetition will become non-trivial when you have 10K evaluations. On the other hand, via an event, the receiver (buildxl, qb, binary log viewer) can choose to deduplicate between multiple project evaluations
- The binary log viewer can only get information via log events, which makes the log events the only way to transport this information. @KirillOsenkov
Implementation wise you could reuse the mechanism that gets used by logging each property reassignment: https://github.com/microsoft/msbuild/blob/master/src/Build/Evaluation/Evaluator.cs#L1400-L1422 (@rainersigwald for opinion)
There was a problem hiding this comment.
I'm not sure how I would propagate events from the ProjectInstance, through the GraphBuilder, up to the ProjectGraph that needs it.
There was a problem hiding this comment.
I think it would require moving the logic to the Evaluator, which already logs a related event, that of property reassignments: https://github.com/microsoft/msbuild/blob/master/src/Build/Evaluation/Evaluator.cs#L1400-L1422.
Then either Bxl/qb or ProjectGraph via some flag would hook up a logger that picks up the event from all the project evaluations and dedupes it.
|
Superseded by #4461. |
7 participants
Implements tracking of environment variables (and uninitialized properties) for graph evaluation.
This allows users of ProjectGraph to better understand what parts of the environment will actually affect the build. This can help reduce the set of environment variables tracked and can potentially reduce unnecessary rebuilds due to environment changes.
Resolves #3432
Related to #2713