Fix: bare dotnet restore doesn't hydrate Node deps for Pcf/ScriptLibrary/CodeApp - #104
Open
TomProkop wants to merge 4 commits into
Open
Fix: bare dotnet restore doesn't hydrate Node deps for Pcf/ScriptLibrary/CodeApp#104TomProkop wants to merge 4 commits into
TomProkop wants to merge 4 commits into
Conversation
…ary/CodeApp A standalone "dotnet restore" sets ExcludeRestorePackageImports=true, so NuGet never imports ordinary PackageReference/buildTransitive-delivered .targets files - including the Tasks package's NodeRestore.targets - during that operation. The per-project-type NodeRestore anchors (_PcfNodeRestore, _ScriptLibraryNodeRestore, _CodeAppNodeRestore) lived in exactly those excluded files, so a bare restore silently skipped Node/rush hydration entirely. Moves the anchor into Sdk.targets, which is imported through the MSBuild SDK-resolver mechanism and is therefore not subject to ExcludeRestorePackageImports. Scoped to Pcf/ScriptLibrary/CodeApp only, since every other project type also references this SDK for unrelated shared targets (e.g. GenerateVersionNumber) but must never attempt Node/rush restore. Two additional issues surfaced during real restore testing against a live PCF control, both fixed here: - $(NuGetPackageRoot) is not reliably populated in the nested MSBuild evaluation NuGet uses internally to build the restore graph, silently preventing the anchor from ever firing. Sdk.props now derives the NuGet package cache root from its own resolved file location instead. - On a brand new machine/cache, the Tasks package isn't downloaded yet when the anchor's nested evaluation runs, so Node deps were never hydrated by that restore at all. A second hook, AfterTargets="Restore", re-invokes the anchor in a freshly re-evaluated nested build once the outer restore has finished downloading packages. Validated end-to-end against FileExplorer (a real, complex PCF control) with disposable local test packages: a single cold-cache "dotnet restore" now hydrates node_modules via rush, a warm-cache restore is idempotent, a normal dotnet build has no duplicate-import warnings, and a Solution-type project restore correctly triggers zero Node/rush activity for itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sdk.targets previously mixed four unrelated concerns in one file: Microsoft.NET.Sdk passthrough, per-project-type PackageReference wiring, Git-versioning defaults, and the NodeRestore restore-time anchor. Split into: - Sdk.targets: thin entry point, imports the three files below. - Sdk.PackageReference.targets: wires in the per-project-type Dataverse package based on $(ProjectType). - Sdk.GitVersioning.targets: Git-based version-number defaults + ResolveGitBranch. - Sdk.NodeRestore.targets: the restore-time Node dependency hydration anchor, with a top-of-file note clarifying it's restore-time only - build-time Node delegation lives in each project type's own .targets file and the Tasks package's NodeBuild targets. Pure reorganization, no logic changes. Validated: dotnet build 0 warnings/0 errors; cold-cache restore against a disposable FileExplorer test project hydrates node_modules via rush; warm restore is idempotent (~2s, no-op); full dotnet build succeeds with 0 warnings (no MSB4011 duplicate-import regression); Solution-type negative test confirms NodeRestore never fires for non-Node project types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follow-up to the Sdk.targets split - the one-line comments in Pcf/ScriptLibrary/CodeApp's own .targets files pointed at Sdk.targets, which no longer directly contains _NodeRestoreAnchor after the split. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A standalone
dotnet restoresetsExcludeRestorePackageImports=true, which makes NuGet skip importing ordinary PackageReference/buildTransitive-delivered.targetsfiles entirely - not just skip running their targets, the files aren't even parsed. The per-project-type NodeRestore anchors (_PcfNodeRestore,_ScriptLibraryNodeRestore,_CodeAppNodeRestore) lived in exactly those excluded files, so a baredotnet restoresilently never hydrated Node/rush dependencies for PCF/ScriptLibrary/CodeApp projects - see build https://dev.azure.com/thenetworg/INT0015/_build/results?buildId=64755, where the restore step completed in 23 seconds without ever touchingcommon/temp/node_modules.Fix
Moves the anchor into
Sdk.targets, which is resolved through the MSBuild SDK-resolver mechanism and is therefore not subject toExcludeRestorePackageImports. Scoped toPcf/ScriptLibrary/CodeApponly - every other project type (Plugin, WorkflowActivity, Solution, PDPackage, GenPage, ...) also references this SDK for unrelated shared targets (e.g.GenerateVersionNumber), but must never attempt Node/rush restore.RunNodeBuild=falsestill opts a project out explicitly.Two additional issues surfaced during real restore testing against a live PCF control, both fixed here:
$(NuGetPackageRoot)is not reliably populated in the nested MSBuild evaluation NuGet uses internally to build the restore graph (_GenerateRestoreGraphProjectEntry), silently preventing the anchor's cross-package import from ever resolving.Sdk.propsnow derives the NuGet package cache root from its own resolved file location instead (the same technique already used forTALXISDevKitDataversePackageVersion).ExcludeRestorePackageImportsonly affects the restore operation. A second hook,AfterTargets="Restore", re-invokes the anchor in a freshly re-evaluated nested build once the outer restore has finished downloading packages.MSB4011) that would otherwise appear on every normaldotnet buildis avoided via a marker property set by the Tasks package's ownbuildTransitiveproxy.Validation
Tested end-to-end against
FileExplorer(a real, complex PCF control) using disposable local test packages, with NuGet cache fully cleared between runs:dotnet restorenow hydratesnode_modulesviarush update.rush updateno-ops in ~0.2s).dotnet build/publish afterward succeeds with 0 warnings, 0 errors (no duplicate-import noise).Solution-type project triggers zero Node/rush activity for itself (only for its Node-type project references, as expected).Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com