Enforce CI properties via Directory.Build.targets temp file #77
Conversation
Generates a temporary Directory.Build.targets file that checks for properties that should not be set, and sets good defaults for CI builds (indicated by the --build-number flag).
There was a problem hiding this comment.
Pull request overview
This PR implements a mechanism to inject custom MSBuild logic at runtime without requiring projects to add a dependency on Faithlife.Build. It creates a temporary Directory.Build.targets file that is injected via the DirectoryBuildTargetsPath MSBuild property, which enforces CI build properties when BuildNumber is set and issues warnings for properties that shouldn't be set explicitly.
Changes:
- Added
Runtime.Directory.Build.targetsfile that enforces CI properties and warns about unnecessary explicit property settings - Embedded the targets file as a resource in the Faithlife.Build assembly
- Created
RuntimeTargetsFilestruct to manage the lifecycle of the temporary targets file - Integrated the runtime targets file into restore, build, and package operations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/Faithlife.Build/Runtime.Directory.Build.targets | New MSBuild targets file that imports the real Directory.Build.targets, enforces CI properties when BuildNumber is set, and warns about explicit property settings |
| src/Faithlife.Build/Faithlife.Build.csproj | Embeds the Runtime.Directory.Build.targets file as a resource |
| src/Faithlife.Build/DotNetBuild.cs | Adds RuntimeTargetsFile struct to extract and manage the temporary targets file, integrates it into restore/build/package targets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| <!-- Check if properties were already set --> | ||
| <PropertyGroup> | ||
| <_FaithlifeBuildAllowedOutputExtensionsInPackageBuildOutputFolderContainsPdb Condition=" $(AllowedOutputExtensionsInPackageBuildOutputFolder.Contains('.pdb')) ">true</_FaithlifeBuildAllowedOutputExtensionsInPackageBuildOutputFolderContainsPdb> |
There was a problem hiding this comment.
The check on line 14 only detects if AllowedOutputExtensionsInPackageBuildOutputFolder already contains .pdb, but the warning message on line 35 suggests that ANY explicit setting of this property should trigger a warning. Consider checking if the property is set at all (non-empty) rather than checking if it contains .pdb: Condition=" '$(AllowedOutputExtensionsInPackageBuildOutputFolder)' != '' ".
| <_FaithlifeBuildAllowedOutputExtensionsInPackageBuildOutputFolderContainsPdb Condition=" $(AllowedOutputExtensionsInPackageBuildOutputFolder.Contains('.pdb')) ">true</_FaithlifeBuildAllowedOutputExtensionsInPackageBuildOutputFolderContainsPdb> | |
| <_FaithlifeBuildAllowedOutputExtensionsInPackageBuildOutputFolderContainsPdb Condition=" '$(AllowedOutputExtensionsInPackageBuildOutputFolder)' != '' ">true</_FaithlifeBuildAllowedOutputExtensionsInPackageBuildOutputFolderContainsPdb> |
There was a problem hiding this comment.
This setting defaults to .dll; .exe; .winmd; .json; .pri; .xml. The presence of .pdb is a fairly strong indication that the user has augmented the property, and it's the addition that should be removed.
| { | ||
| public static RuntimeTargetsFile Create(DotNetBuildSettings settings) | ||
| { |
There was a problem hiding this comment.
The settings parameter is not used in this method. Consider removing it or documenting why it's included for future extensibility.
| { | |
| public static RuntimeTargetsFile Create(DotNetBuildSettings settings) | |
| { | |
| { | |
| /// <summary> | |
| /// Creates a <see cref="RuntimeTargetsFile"/>. The <paramref name="settings"/> parameter is | |
| /// accepted for API consistency and future extensibility but is not currently used. | |
| /// </summary> | |
| /// <param name="settings">Build settings reserved for future use.</param> | |
| public static RuntimeTargetsFile Create(DotNetBuildSettings settings) | |
| { | |
| // Intentionally unused for now; reserved for future configuration-based behavior. | |
| _ = settings; |
There was a problem hiding this comment.
This used to be TryCreate and it would return null if GetBuildNumber was set, omitting the file entirely. We now include it in all builds so this is vestigial and should be fixed.
| private readonly struct RuntimeTargetsFile : IDisposable | ||
| { | ||
| public static RuntimeTargetsFile Create(DotNetBuildSettings settings) | ||
| { | ||
| var tempPath = Path.Combine(Path.GetTempPath(), $"FaithlifeBuild.{Guid.NewGuid().ToString("n")[^8..]}.targets"); | ||
|
|
||
| var assembly = Assembly.GetExecutingAssembly(); | ||
| var resourceName = "Faithlife.Build.Runtime.Directory.Build.targets"; | ||
| using var resourceStream = assembly.GetManifestResourceStream(resourceName) ?? throw new BuildException($"Embedded resource '{resourceName}' not found."); | ||
| using var fileStream = File.Create(tempPath); | ||
| resourceStream.CopyTo(fileStream); | ||
|
|
||
| return new(tempPath); | ||
| } | ||
|
|
||
| public RuntimeTargetsFile(string targetsFilePath) | ||
| { | ||
| TargetsFilePath = targetsFilePath; | ||
| } | ||
|
|
||
| public string TargetsFilePath { get; } | ||
|
|
||
| public string GetBuildArg() => $"-p:DirectoryBuildTargetsPath={TargetsFilePath}"; | ||
|
|
||
| public void Dispose() | ||
| { | ||
| if (File.Exists(TargetsFilePath)) | ||
| { | ||
| try | ||
| { | ||
| File.Delete(TargetsFilePath); | ||
| } | ||
| catch (Exception) | ||
| { | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new RuntimeTargetsFile struct and its integration with build/restore/package targets lack test coverage. Consider adding tests to verify: 1) the embedded resource is correctly extracted to a temp file, 2) the temp file is properly cleaned up on disposal, 3) the GetBuildArg method returns the correct MSBuild argument format, and 4) the file extraction works correctly when the resource is missing.
| return infos.Count == 0 ? "" : $" ({string.Join(", ", infos)})"; | ||
| } | ||
|
|
||
| private readonly struct RuntimeTargetsFile : IDisposable |
There was a problem hiding this comment.
The RuntimeTargetsFile struct lacks XML documentation comments. Consider adding documentation to explain its purpose, lifecycle, and usage pattern, particularly noting that it should be disposed promptly after the MSBuild invocation completes.
Setting it in Directory.Build.targets may be too late for it to take effect.
Co-authored-by: Copilot <[email protected]>
This allows them to be included for a local dev 'package' task.
Use
DirectoryBuildTargetsPathas an extension mechanism to inject a customDirectory.Build.targetsfile that enforces the properties we want to be set for CI builds. Also issues warnings for the properties that can be detected to be set unnecessarily already.We can now inject custom MSBuild logic at runtime without having to add a
buildTransitivefile to the Faithlife.Build NuGet package and also make every project that we build add a reference (or<GlobalPackageReference>) to Faithlife.Build. (Probably to a separate, new NuGet package because we wouldn't actually want to make Faithlife.Build a dependency.)