Skip to content

Commit 2dac5e1

Browse files
dfedermdfederm
committed
PR comments
1 parent cf5d6c4 commit 2dac5e1

34 files changed

+159
-91
lines changed

‎src/Build/BackEnd/BuildManager/BuildManager.cs‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -562,6 +562,12 @@ public void BeginBuild(BuildParameters parameters)
562562
#if FEATURE_REPORTFILEACCESSES
563563
if (_buildParameters.ReportFileAccesses)
564564
{
565+
// To properly report file access, we need to disable the in-proc node which won't be detoured.
566+
_buildParameters.DisableInProcNode = true;
567+
568+
// Node reuse must be disabled as future builds will not be able to listen to events raised by detours.
569+
_buildParameters.EnableNodeReuse = false;
570+
565571
_componentFactories.ReplaceFactory(BuildComponentType.NodeLauncher, DetouredNodeLauncher.CreateComponent);
566572
}
567573
#endif
@@ -576,11 +582,16 @@ public void BeginBuild(BuildParameters parameters)
576582

577583
InitializeCaches();
578584

585+
#if FEATURE_REPORTFILEACCESSES
579586
var fileAccessManager = ((IBuildComponentHost)this).GetComponent(BuildComponentType.FileAccessManager) as IFileAccessManager;
587+
#endif
588+
580589
_projectCacheService = new ProjectCacheService(
581590
this,
582591
loggingService,
592+
#if FEATURE_REPORTFILEACCESSES
583593
fileAccessManager,
594+
#endif
584595
_configCache,
585596
_buildParameters.ProjectCacheDescriptor);
586597

@@ -2398,8 +2409,9 @@ private void HandleResult(int node, BuildResult result)
23982409
if (_buildSubmissions.TryGetValue(result.SubmissionId, out BuildSubmission buildSubmission))
23992410
{
24002411
// The result may be associated with the build submission due to it being the submission which
2401-
// caused the build, but not the actual request which was used with the build submission. Ensure
2402-
// only the actual submission's request is considered.
2412+
// caused the build, but not the actual request which was originally used with the build submission.
2413+
// ie. it may be a dependency of the "root-level" project which is associated with this submission, which
2414+
// isn't what we're looking for. Ensure only the actual submission's request is considered.
24032415
if (buildSubmission.BuildRequest != null
24042416
&& buildSubmission.BuildRequest.ConfigurationId == configuration.ConfigurationId
24052417
&& _projectCacheService.ShouldUseCache(configuration))

‎src/Build/BackEnd/BuildManager/BuildParameters.cs‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -811,20 +811,7 @@ public string OutputResultsCacheFile
811811
public bool ReportFileAccesses
812812
{
813813
get => _reportFileAccesses;
814-
set
815-
{
816-
_reportFileAccesses = value;
817-
818-
// TODO dfederm: What if either of these are set after ReportFileAccesses is? Do we need to move this elsewhere?
819-
if (_reportFileAccesses)
820-
{
821-
// To properly report file access, we need to disable the in-proc node which won't be detoured.
822-
DisableInProcNode = true;
823-
824-
// Node reuse must be disabled as future builds will not be able to listen to events raised by detours.
825-
EnableNodeReuse = false;
826-
}
827-
}
814+
set => _reportFileAccesses = value;
828815
}
829816
#endif
830817

‎src/Build/BackEnd/Components/BuildComponentFactoryCollection.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,9 @@ public void RegisterDefaultFactories()
8383
// SDK resolution
8484
_componentEntriesByType[BuildComponentType.SdkResolverService] = new BuildComponentEntry(BuildComponentType.SdkResolverService, MainNodeSdkResolverService.CreateComponent, CreationPattern.Singleton);
8585

86+
#if FEATURE_REPORTFILEACCESSES
8687
_componentEntriesByType[BuildComponentType.FileAccessManager] = new BuildComponentEntry(BuildComponentType.FileAccessManager, FileAccessManager.CreateComponent, CreationPattern.Singleton);
88+
#endif
8789
}
8890

8991
/// <summary>

‎src/Build/BackEnd/Components/Communications/DetouredNodeLauncher.cs‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -71,13 +71,9 @@ public Process Start(string msbuildLocation, string commandLineArgs, int nodeId)
7171

7272
string exeName = msbuildLocation;
7373

74-
#if RUNTIME_TYPE_NETCORE || MONO
75-
// Mono automagically uses the current mono, to execute a managed assembly
76-
if (!NativeMethodsShared.IsMono)
77-
{
78-
// Run the child process with the same host as the currently-running process.
79-
exeName = CurrentHost.GetCurrentHost();
80-
}
74+
#if RUNTIME_TYPE_NETCORE
75+
// Run the child process with the same host as the currently-running process.
76+
exeName = CurrentHost.GetCurrentHost();
8177
#endif
8278

8379
var eventListener = new DetoursEventListener(_fileAccessManager, nodeId);
@@ -117,7 +113,7 @@ public Process Start(string msbuildLocation, string commandLineArgs, int nodeId)
117113
// needed for logging process arguments when a new process is invoked; see DetoursEventListener.cs
118114
info.FileAccessManifest.ReportProcessArgs = true;
119115

120-
// By default, Domino sets the timestamp of all input files to January 1, 1970
116+
// By default, BuildXL sets the timestamp of all input files to January 1, 1970
121117
// This breaks some tools like Robocopy which will not copy a file to the destination if the file exists at the destination and has a timestamp that is more recent than the source file
122118
info.FileAccessManifest.NormalizeReadTimestamps = false;
123119

‎src/Build/BackEnd/Components/FileAccesses/FileAccessManager.cs‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
#if FEATURE_REPORTFILEACCESSES
45
using System;
56
using System.Collections.Concurrent;
67
using System.IO;
8+
using System.Runtime.Versioning;
79
using System.Threading;
810
using Microsoft.Build.BackEnd;
911
using Microsoft.Build.Execution;
@@ -19,6 +21,7 @@ private record Handlers(Action<BuildRequest, FileAccessData> FileAccessHander, A
1921
// In order to synchronize between the node communication and the file access reporting, a special file access
2022
// is used to mark when the file accesses should be considered complete. Only after both this special file access is seen
2123
// and the build result is reported can plugins be notified about project completion.
24+
// NOTE! This is currently Windows-specific and will need to change once this feature is opened up to more scenarios.
2225
private static readonly string FileAccessCompletionPrefix = BuildParameters.StartupDirectory[0] + @":\{MSBuildFileAccessCompletion}\";
2326

2427
private IScheduler? _scheduler;
@@ -67,8 +70,8 @@ public void ReportFileAccess(FileAccessData fileAccessData, int nodeId)
6770
}
6871
else if (_tempDirectory != null && fileAccessPath.StartsWith(_tempDirectory))
6972
{
70-
// Ignore the temp directory as these are related to internal MSBuild functionality and not always directly related to the execution of the project itself,
71-
// so should not be exposed to handlers.
73+
// Ignore MSBuild's temp directory as these are related to internal MSBuild functionality and not always directly related to the execution of the project itself,
74+
// so should not be exposed to handlers. Note that this is not %TEMP% but instead a subdir under %TEMP% which is only expected to be used by MSBuild.
7275
return;
7376
}
7477
else
@@ -134,6 +137,9 @@ private void UnregisterHandlers(Handlers handlersToRemove)
134137
}
135138
}
136139

140+
// The [SupportedOSPlatform] attribute is a safeguard to ensure that the comment on FileAccessCompletionPrefix regarding being Windows-only gets addressed.
141+
// [SupportedOSPlatform] doesn't apply to fields, so using it here as a reasonable proxy.
142+
[SupportedOSPlatform("windows")]
137143
public static void NotifyFileAccessCompletion(int globalRequestId)
138144
{
139145
// Make a dummy file access to use as a notification that the file accesses should be completed for a project.
@@ -179,3 +185,4 @@ public void Dispose()
179185
}
180186
}
181187
}
188+
#endif

‎src/Build/BackEnd/Components/FileAccesses/IFileAccessManager.cs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
#if FEATURE_REPORTFILEACCESSES
45
using System;
56
using System.Threading;
67
using Microsoft.Build.BackEnd;
@@ -14,11 +15,12 @@ internal interface IFileAccessManager
1415

1516
void ReportProcess(ProcessData processData, int nodeId);
1617

17-
// Note: HandlerRegistration is exposed directly instead of IDisposable to avoid boxing.
18+
// Note: The return type of FileAccessManager.HandlerRegistration is exposed directly instead of IDisposable to avoid boxing.
1819
FileAccessManager.HandlerRegistration RegisterHandlers(
1920
Action<BuildRequest, FileAccessData> fileAccessHandler,
2021
Action<BuildRequest, ProcessData> processHandler);
2122

2223
void WaitForFileAccessReportCompletion(int globalRequestId, CancellationToken cancellationToken);
2324
}
2425
}
26+
#endif

‎src/Build/BackEnd/Components/FileAccesses/OutOfProcNodeFileAccessManager.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
#if FEATURE_REPORTFILEACCESSES
45
using System;
56
using System.Threading;
67
using Microsoft.Build.BackEnd;
@@ -59,3 +60,4 @@ public void WaitForFileAccessReportCompletion(int globalRequestId, CancellationT
5960
throw new NotImplementedException("This method should not be called in OOP nodes.");
6061
}
6162
}
63+
#endif

‎src/Build/BackEnd/Components/IBuildComponentHost.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,10 +131,12 @@ internal enum BuildComponentType
131131
/// </summary>
132132
SdkResolverService,
133133

134+
#if FEATURE_REPORTFILEACCESSES
134135
/// <summary>
135136
/// The component which is the sink for file access reports and forwards reports to other components.
136137
/// </summary>
137138
FileAccessManager,
139+
#endif
138140

139141
/// <summary>
140142
/// The component which launches new MSBuild nodes.

‎src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs‎

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,9 @@ internal sealed class ProjectCacheService : IAsyncDisposable
3636
private readonly BuildManager _buildManager;
3737
private readonly IBuildComponentHost _componentHost;
3838
private readonly ILoggingService _loggingService;
39+
#if FEATURE_REPORTFILEACCESSES
3940
private readonly IFileAccessManager _fileAccessManager;
41+
#endif
4042
private readonly IConfigCache _configCache;
4143

4244
private readonly ProjectCacheDescriptor? _globalProjectCacheDescriptor;
@@ -53,7 +55,9 @@ internal sealed class ProjectCacheService : IAsyncDisposable
5355
private record struct ProjectCachePlugin(
5456
string Name,
5557
ProjectCachePluginBase? Instance,
58+
#if FEATURE_REPORTFILEACCESSES
5659
FileAccessManager.HandlerRegistration? HandlerRegistration,
60+
#endif
5761
ExceptionDispatchInfo? InitializationException = null);
5862

5963
/// <summary>
@@ -72,14 +76,18 @@ private DefaultMSBuildFileSystem()
7276
public ProjectCacheService(
7377
BuildManager buildManager,
7478
ILoggingService loggingService,
79+
#if FEATURE_REPORTFILEACCESSES
7580
IFileAccessManager fileAccessManager,
81+
#endif
7682
IConfigCache configCache,
7783
ProjectCacheDescriptor? globalProjectCacheDescriptor)
7884
{
7985
_buildManager = buildManager;
8086
_componentHost = buildManager;
8187
_loggingService = loggingService;
88+
#if FEATURE_REPORTFILEACCESSES
8289
_fileAccessManager = fileAccessManager;
90+
#endif
8391
_configCache = configCache;
8492
_globalProjectCacheDescriptor = globalProjectCacheDescriptor;
8593
}
@@ -203,7 +211,13 @@ private async Task<ProjectCachePlugin> CreateAndInitializePluginAsync(
203211
}
204212
catch (Exception e)
205213
{
206-
return new ProjectCachePlugin(pluginTypeName, Instance: null, HandlerRegistration: null, ExceptionDispatchInfo.Capture(e));
214+
return new ProjectCachePlugin(
215+
pluginTypeName,
216+
Instance: null,
217+
#if FEATURE_REPORTFILEACCESSES
218+
HandlerRegistration: null,
219+
#endif
220+
ExceptionDispatchInfo.Capture(e));
207221
}
208222
finally
209223
{
@@ -234,7 +248,11 @@ await pluginInstance.BeginBuildAsync(
234248
ProjectCacheException.ThrowForErrorLoggedInsideTheProjectCache("ProjectCacheInitializationFailed");
235249
}
236250

237-
FileAccessManager.HandlerRegistration handlerRegistration = _fileAccessManager.RegisterHandlers(
251+
#if FEATURE_REPORTFILEACCESSES
252+
FileAccessManager.HandlerRegistration? handlerRegistration = null;
253+
if (_componentHost.BuildParameters.ReportFileAccesses)
254+
{
255+
handlerRegistration = _fileAccessManager.RegisterHandlers(
238256
(buildRequest, fileAccessData) =>
239257
{
240258
// TODO: Filter out projects which do not configure this plugin
@@ -247,12 +265,26 @@ await pluginInstance.BeginBuildAsync(
247265
FileAccessContext fileAccessContext = GetFileAccessContext(buildRequest);
248266
pluginInstance.HandleProcess(fileAccessContext, processData);
249267
});
268+
}
269+
#endif
250270

251-
return new ProjectCachePlugin(pluginTypeName, pluginInstance, handlerRegistration);
271+
return new ProjectCachePlugin(
272+
pluginTypeName,
273+
pluginInstance,
274+
#if FEATURE_REPORTFILEACCESSES
275+
handlerRegistration,
276+
#endif
277+
InitializationException: null);
252278
}
253279
catch (Exception e)
254280
{
255-
return new ProjectCachePlugin(pluginTypeName, Instance: null, HandlerRegistration: null, ExceptionDispatchInfo.Capture(e));
281+
return new ProjectCachePlugin(
282+
pluginTypeName,
283+
Instance: null,
284+
#if FEATURE_REPORTFILEACCESSES
285+
HandlerRegistration: null,
286+
#endif
287+
ExceptionDispatchInfo.Capture(e));
256288
}
257289
finally
258290
{
@@ -773,10 +805,12 @@ public async ValueTask DisposeAsync()
773805
return;
774806
}
775807

808+
#if FEATURE_REPORTFILEACCESSES
776809
if (plugin.HandlerRegistration.HasValue)
777810
{
778811
plugin.HandlerRegistration.Value.Dispose();
779812
}
813+
#endif
780814

781815
MSBuildEventSource.Log.ProjectCacheEndBuildStart(plugin.Name);
782816
try

‎src/Build/BackEnd/Node/OutOfProcNode.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,9 +156,12 @@ public OutOfProcNode()
156156
OutOfProcNodeSdkResolverServiceFactory sdkResolverServiceFactory = new OutOfProcNodeSdkResolverServiceFactory(SendPacket);
157157
((IBuildComponentHost)this).RegisterFactory(BuildComponentType.SdkResolverService, sdkResolverServiceFactory.CreateInstance);
158158
_sdkResolverService = (this as IBuildComponentHost).GetComponent(BuildComponentType.SdkResolverService) as ISdkResolverService;
159+
160+
#if FEATURE_REPORTFILEACCESSES
159161
((IBuildComponentHost)this).RegisterFactory(
160162
BuildComponentType.FileAccessManager,
161163
(componentType) => OutOfProcNodeFileAccessManager.CreateComponent(componentType, SendPacket));
164+
#endif
162165

163166
if (s_projectRootElementCacheBase == null)
164167
{

0 commit comments

Comments
 (0)