Streamline docs sdk authoring and end-user experience #13
Conversation
|
|
||
| var apiOutputPath = Path.Combine(Context.DocumentationRootPath, Context.ApiReferencePath); | ||
| Console.WriteLine($"📝 Rendering documentation to: {apiOutputPath}"); | ||
| // Console.WriteLine($"📝 Rendering documentation to: {apiOutputPath}"); |
There was a problem hiding this comment.
direct console usage in internal libraries is very bad from the MSBuild perspective - you don't own stdout and should instead route all such logging through the MSBuild ILogger interfaces. Typically what this looks like for well-layered apps is using the M.E.L.ILogger internally, and making a small bridge adapter from M.E.L.ILogger to MSBuild's ILogger interface.
| @@ -1,54 +0,0 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
This whole project went away because you can use a few features of the .NET SDK to both develop the tasks and bundle them in the SDK package altogether.
| try | ||
| { | ||
| // Create an evaluated project to get computed property values | ||
| var project = new Project(projectFile, globalProperties, null, projectCollection); |
There was a problem hiding this comment.
doing anything like this in a Task is very bad. first for dll-dependency-management reasons, but more importantly because none of the evaluation work you've done in the Task implementation now has the chance to be 'served' from the overall build execution context. Generally in MSBuild, when we need data from other Projects we
- do it in MSBuild XML Logic
- establish a link between the currently-executing thing and the 'child' projects
- request data from those child projects via calling Targets
this allows the MSBuild engine to a) distribute the work across any available worker nodes and b) re-use already-existing MSBuild evaluations (what happens when you construct a new Project instance) that the engine already knows about.
| var projectExtensions = new[] { "*.csproj", "*.vbproj", "*.fsproj" }; | ||
| var projectFiles = projectExtensions | ||
| .SelectMany(ext => Directory.GetFiles(SolutionDir, ext, SearchOption.AllDirectories)) | ||
| .Where(file => ExcludePatterns is null || !ExcludePatterns.Any(pattern => | ||
| Path.GetFileNameWithoutExtension(file).Contains(pattern.Replace("*", ""), StringComparison.OrdinalIgnoreCase))) | ||
| .ToArray(); |
There was a problem hiding this comment.
generally speaking, when you're doing file-probing and discovery in a Task that's a signal that the Task isn't fitting in to the MSBuild model. file searching/globbing/etc should be done from the MSBuild engine so that it can trace dependencies better.
| @@ -1,15 +1,17 @@ | |||
| <Project Sdk="Microsoft.Build.NoTargets/3.7.0"> | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
Moving to the 'base' .NET SDK allows us to compile the Tasks in this project.
| <PropertyGroup> | ||
| <!-- SDK packages don't compile code --> | ||
| <TargetFramework>netstandard2.0</TargetFramework> | ||
| <TargetFrameworks>net8.0;net472</TargetFrameworks> |
There was a problem hiding this comment.
You only actually used the net472 and net8.0 builds of your Tasks in your Targets code, so I slimmed things down to just those two TFMs. The code that is below will easily scale to more TFMs if you want to add them, though.
|
|
||
| <!-- Suppress dependencies when packing --> | ||
| <SuppressDependenciesWhenPacking>true</SuppressDependenciesWhenPacking> | ||
| <TargetsForTfmSpecificContentInPackage>$(TargetsForTfmSpecificContentInPackage);AddPerTFMDocsTasksToPackage</TargetsForTfmSpecificContentInPackage> |
There was a problem hiding this comment.
This is the magic - the packaging logic you had before was reconstructing part of what NuGet already gives you. When you pack a nuget package, they give you some extension points for easily and correctly adding TFM-specific assets to the package. Your Task dlls and their dependencies are an example of such TFM-specific assets, so we leverage this functionality now.
| <PackageReference Include="EasyAF.MSBuild" Version="3.*-*" /> | ||
| <PackageReference Include="Microsoft.Build.Framework" Version="17.*" PrivateAssets="all" /> | ||
| <PackageReference Include="Microsoft.Build.Utilities.Core" Version="17.*" PrivateAssets="all" ExcludeAssets="runtime" /> | ||
| <PackageReference Include="System.Collections.Immutable" Version="9.*" PrivateAssets="all" /> | ||
|
|
||
| <!-- Microsoft.CodeAnalysis.CSharp.Workspaces needs to be available for the task --> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.*" /> |
There was a problem hiding this comment.
nit: without a lock file, all of these dependencies are nondeterministic. This is bad for consumers of your MSBuild SDK. You should either use a lockfile with these or move to single-version references here so that consumers of your SDK know what they're getting.
| <Target Name="AddPerTFMDocsTasksToPackage" Returns="@(TfmSpecificPackageFile)" DependsOnTargets="ResolveProjectReferences"> | ||
| <PropertyGroup> | ||
| <_TargetsForReferenceOutputs>ReferenceCopyLocalPathsOutputGroup;BuiltProjectOutputGroup</_TargetsForReferenceOutputs> | ||
| </PropertyGroup> | ||
| <!-- for each of our TFMs, we need to get a few kinds of outputs and put them into the package --> | ||
| <MSBuild Projects="$(MSBuildThisFileFullPath)" | ||
| Targets="$(_TargetsForReferenceOutputs)"> | ||
| <Output TaskParameter="TargetOutputs" ItemName="CurrentTFMOutputs" /> | ||
| </MSBuild> | ||
|
|
||
| <ItemGroup> | ||
| <RequiredTaskFiles Include="..\CloudNimble.DotNetDocs.Sdk.Tasks\bin\$(Configuration)\**\*.dll" /> | ||
| <CurrentTFMOutputs Update="@(CurrentTFMOutputs)" PackagePath="tasks\$(TargetFramework)\$([MSBuild]::ValueOrDefault('%(CurrentTFMOutputs.DestinationSubPath)', '%(CurrentTFMOutputs.TargetPath)'))" /> | ||
| <TfmSpecificPackageFile Include="@(CurrentTFMOutputs)" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
the contract nuget has for this TFM-specific packaging target is
- we'll call this Target once per TFM
- you create
TfmSpecificPackagefileItems with thePackagePathattribute set
so we do this by asking MSBuild for what our 'runtime outputs' are for the current project's configuration (remember this is implicitly keyed to a specific TFM) and then we place those runtime outputs at tasks\<TFM>\<file path> programmaticaly instead of hard-coding. This strategy should be resilient to changes to TFMs that you make in the future.
| <Target Name="IncludeNoTargetsInPackage" BeforeTargets="GenerateNuspec"> | ||
| <PropertyGroup> | ||
| <NoTargetsSourcePath>$(PkgMicrosoft_Build_NoTargets)\Sdk</NoTargetsSourcePath> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <NoTargetsFiles Include="$(NoTargetsSourcePath)\**\*" /> | ||
| <None Include="@(NoTargetsFiles)" Pack="true" PackagePath="Sdk\NoTargets\%(RecursiveDir)%(Filename)%(Extension)" /> | ||
| </ItemGroup> | ||
|
|
||
| <Message Text="Including NoTargets SDK files in package from $(NoTargetsSourcePath)" Importance="normal" /> | ||
| </Target> |
There was a problem hiding this comment.
Bundling NoTargets like this is no bueno - at first I moved your SDK to do a direct reference + inclusion of the NoTargets SDK in your docproj targets, but eventually I made your SDK a 'proper' .NET SDK project because we'll end up needed better support for ProjectReferences.
| Log.LogMessage(MessageImportance.High, "📊 Documentation Statistics:"); | ||
| Log.LogMessage(MessageImportance.High, $" 📄 Documentation type: {DocumentationType}"); | ||
| Log.LogMessage(MessageImportance.High, $" 📦 Assemblies processed: {assemblyPairs.Count}"); | ||
|
|
||
| if (generatedFiles.Count > 0) | ||
| { | ||
| Log.LogMessage(MessageImportance.High, $" 📝 Files generated: {generatedFiles.Distinct().Count()}"); | ||
| } |
There was a problem hiding this comment.
This logging could come back - it was just cluttering the view for me while I worked.
| <!-- Inherit from NoTargets SDK for no-compile behavior --> | ||
| <UsingMicrosoftNoTargetsSdk>true</UsingMicrosoftNoTargetsSdk> | ||
|
|
There was a problem hiding this comment.
This goes away because we're no longer based on the NoTargets SDK
| <BaseOutputPath Condition="'$(BaseOutputPath)' == ''">$([System.IO.Path]::GetTempPath())$(_SolutionName)\$(MSBuildProjectName)\bin\</BaseOutputPath> | ||
| <IntermediateOutputPath>$(BaseIntermediateOutputPath)</IntermediateOutputPath> | ||
| <OutputPath>$(BaseOutputPath)</OutputPath> | ||
| <MSBuildProjectExtensionsPath>$(BaseIntermediateOutputPath)</MSBuildProjectExtensionsPath> |
There was a problem hiding this comment.
WOAH this is not cool - MSBuildProjectExtensionsPath is a private, very special property. I can only assume the engine didn't let you set this because the dotnet CLI sets it via env var, so it becomes un-overridable.
| <!-- Import from NuGet cache if found --> | ||
| <Import Project="$(_NoTargetsFallbackPath)" | ||
| Condition="!Exists('$(MSBuildThisFileDirectory)NoTargets\Sdk.props') and Exists('$(_NoTargetsFallbackPath)')" /> | ||
| <Import Sdk="Microsoft.NET.Sdk" Project="Sdk.props" /> |
There was a problem hiding this comment.
No we chain directly into the 'base' .NET SDK - but after setting up properties that we need.
| <!-- Import from NuGet cache if found --> | ||
| <Import Project="$(_NoTargetsTargetsFallbackPath)" | ||
| Condition="!Exists('$(MSBuildThisFileDirectory)NoTargets\Sdk.targets') and Exists('$(_NoTargetsTargetsFallbackPath)')" /> | ||
| <Import Sdk="Microsoft.NET.SDK" Project="Sdk.targets" /> |
There was a problem hiding this comment.
And similarly we can chain to the base .NET SDK's targets in a nice manner - this is what 'value added' SDKs like the Aspire SDK do, too.
| <_DotNetDocsTasksFolder Condition="'$(_DotNetDocsTasksFolder)' == ''">$(MSBuildThisFileDirectory)..\tasks\</_DotNetDocsTasksFolder> | ||
| <_DotNetDocsTasksAssembly >$(_DotNetDocsTasksFolder)$(_DotNetDocsTaskFramework)\CloudNimble.DotNetDocs.Sdk.dll</_DotNetDocsTasksAssembly> | ||
|
|
||
| <PrepareProjectReferencesDependsOn>AssignDocumentationProjectReferences;$(PrepareProjectReferencesDependsOn)</PrepareProjectReferencesDependsOn> |
There was a problem hiding this comment.
Now it's time to talk about the biggest conceptual change to the way your SDK works:
Before it didn't enforce that the things it was trying to generate docs for were actually built. This is because there were no ProjectReferences between the docproj and the rest of the repo. That is changed - now there's a Target (AssignDocumentationProjectReferences) whose job is to discover and create synthetic projectreferences for the docproj - because I'm intuiting that you didn't want the docproj cluttered with a bunch of ProjectReference elements.
These synthetic ProjectReferences help the .NET SDK enforce a) ordering and b) built-ed-ness before documentation generation occurs. This process is generally very quick because of MSbuild's incrementality.
So we
a) make the implied project references
b) use MSBuild to discover which ones are doc-able
c) use MSBuild to get the inputs for those projects for the doc generation process
d) generate the docs
and all of it plays nicely with incrementality, while ensuring that someone building your docproj never fails a build or misses a doc input.
| and dlls from the BuiltProjectOutputGroup Target. HOWEVER these only exist on single-TFM projects, so | ||
| first we need to ensure that we have 'negotiated' the correct TFM (and other properties) for each project. | ||
| --> | ||
| <Target Name="DiscoverDocumentedProjects" Condition="'$(GenerateDocumentation)' == 'true'" DependsOnTargets="PrepareProjectReferences"> |
There was a problem hiding this comment.
This is the fun Target - before you were doing some internal logic about which TFM to choose for your projects when they were multi-TFM. Because of the change I've made, the .NET SDK has already computed that for you - the data is on the _MSBuildProjectReferenceExistent MSBuild Items, which were created from your ProjectReferences.
So now we can use this data to get information about the doc outputs and dll outputs of the projects to determine if those projects can even be doc'd. The SDK won't make xmldocs for a project unless it has explicitly opted into them, so their presence is a good filter.
We ask MSBuild for which projects have docs, then use those projects to ask for the dll outputs, and then we have all of the data we need to generate - and we got it all without doing any additional work, because the engine already had built and computed these details about these projects.
|
|
||
| <!-- Generate documentation from existing assemblies --> | ||
| <!-- Generate documentation from existing assemblies. TODO: is it possible to compute the set of outputs that are expected? That would help incrementality a ton --> | ||
| <Target Name="GenerateDocumentation" |
There was a problem hiding this comment.
Generation actually stays pretty much the same.
| </CloudNimble.DotNetDocs.Sdk.Tasks.GenerateDocumentationTask> | ||
|
|
||
| <ItemGroup> | ||
| <FileWrites Include="@(GeneratedDocumentationFiles)" /> |
There was a problem hiding this comment.
adding 'FileWrites' makes dotnet clean work with your doc generation. If you could 'precompute' the list of expected outputs from doc generation, then you could make doc generation zero-cost if none of the inputs and none of the outputs have changed. The power of MSBuild!
| <Project Sdk="CloudNimble.DotNetDocs.Sdk/1.0.0"> | ||
| <Project> | ||
|
|
||
| <Import Project="..\src\CloudNimble.DotNetDocs.Sdk\Sdk\Sdk.props" /> |
There was a problem hiding this comment.
Here's an example of how to test local SDKs. An SDK is just a props import at the logical top of the file, and a targets import at the logical bottom of the file. So you can do that yourself and get pretty much the same experience.
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <_DotNetDocsTasksFolder>$(MSBuildThisFileDirectory)..\src\CloudNimble.DotNetDocs.Sdk\bin\Debug\</_DotNetDocsTasksFolder> |
There was a problem hiding this comment.
This is the only slightly-weird thing. Since you're loading the props and targets from the repo and not from an SDK package, it becomes necessary to tell your SDK targets where the matching tasks implementations are.
1 participant
Here's that PR I was mentioning - I don't have the full write-up but I'll do a quick drive-by review here of the highlights.