Skip to content

Commit 25b15be

Browse files
stephentoubCopilot
andauthored
Honor preinstalled CLI path in .NET MSBuild targets (#921) (#1318)
* Honor preinstalled CLI path in .NET MSBuild targets (#921) The `CopilotSkipCliDownload=true` escape hatch also gated the copy and register targets, so consumers behind authenticated npm mirrors had no way to point the build at a pre-downloaded copilot CLI. `_CopilotCliBinaryPath` was also unconditionally reassigned inside each target, so even setting it via a global property had no effect. Add a public `CopilotCliBinaryPath` property that: * suppresses `_DownloadCopilotCli` * still runs `_CopyCopilotCliToOutput` and `_RegisterCopilotCliForCopy` so the supplied binary is placed at the canonical `runtimes/<rid>/native/copilot[.exe]` path that `Client.GetBundledCliPath` searches at runtime * fails the build with an actionable `<Error>` if the path is missing Switch the `<Copy>` task from `DestinationFolder` to `DestinationFiles` so off-spec source filenames are normalized to `copilot[.exe]` on copy. Add `dotnet/test/Unit/MSBuildTargetsTests.cs` -- the first MSBuild-target test in the repo -- with 5 integration tests that import the real targets file into a throwaway csproj and shell out to `dotnet build` to validate the four scenarios (preinstalled, preinstalled + skip, skip only, missing path) end-to-end. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback on MSBuildTargetsTests - Also drop RuntimeIdentifier from the subprocess env (the comment said it but only MSBuildSDKsPath was actually removed), so CI workers that set a RID don't pull the build into a different runtimes folder than ExpectedOutputBinary() expects. - Narrow the two best-effort catch clauses (process.Kill on timeout, and Directory.Delete in Dispose) to the specific exception types those operations actually throw, while keeping the same swallow-and- continue semantics. - Sanitize the caller-supplied fileName in WritePreinstalledBinary via Path.GetFileName so a rooted or directory-containing value can't silently escape preinstallDir via Path.Combine. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent a623d07 commit 25b15be

2 files changed

Lines changed: 336 additions & 12 deletions

File tree

dotnet/src/build/GitHub.Copilot.SDK.targets

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,33 @@
5555
<CopilotCliDownloadTimeout Condition="'$(CopilotCliDownloadTimeout)' == ''">600</CopilotCliDownloadTimeout>
5656
</PropertyGroup>
5757

58-
<!-- Download and extract CLI binary. Set CopilotSkipCliDownload=true to skip if you install the CLI separately. -->
59-
<Target Name="_DownloadCopilotCli" BeforeTargets="BeforeBuild" Condition="'$(CopilotSkipCliDownload)' != 'true' And '$(_CopilotPlatform)' != ''">
58+
<!-- Allow consumers to point the targets at a pre-downloaded Copilot CLI binary
59+
(for example, fetched separately using `npm pack` with authentication for a private
60+
npm feed, since MSBuild's DownloadFile task does not support credentials).
61+
When CopilotCliBinaryPath is set:
62+
- The built-in download target is skipped automatically; no need to also set
63+
CopilotSkipCliDownload=true.
64+
- The copy and register targets still place that binary into the build output
65+
and propagate it to referencing projects.
66+
Example:
67+
<PropertyGroup>
68+
<CopilotCliBinaryPath>$(MSBuildProjectDirectory)\copilot-cli\copilot.exe</CopilotCliBinaryPath>
69+
</PropertyGroup>
70+
-->
71+
<PropertyGroup>
72+
<_CopilotCliBinaryPath Condition="'$(CopilotCliBinaryPath)' != ''">$(CopilotCliBinaryPath)</_CopilotCliBinaryPath>
73+
</PropertyGroup>
74+
75+
<!-- Download and extract CLI binary.
76+
Skipped when CopilotCliBinaryPath is set (consumer supplied the binary)
77+
or when CopilotSkipCliDownload=true (consumer opted out entirely). -->
78+
<Target Name="_DownloadCopilotCli" BeforeTargets="BeforeBuild" Condition="'$(CopilotCliBinaryPath)' == '' And '$(CopilotSkipCliDownload)' != 'true' And '$(_CopilotPlatform)' != ''">
6079
<Error Condition="'$(CopilotCliVersion)' == ''" Text="CopilotCliVersion is not set. The GitHub.Copilot.SDK.props file may be missing from the NuGet package." />
6180

6281
<!-- Compute paths using version (now available) -->
6382
<PropertyGroup>
6483
<_CopilotCacheDir>$(IntermediateOutputPath)copilot-cli\$(CopilotCliVersion)\$(_CopilotPlatform)</_CopilotCacheDir>
65-
<_CopilotCliBinaryPath>$(_CopilotCacheDir)\$(_CopilotBinary)</_CopilotCliBinaryPath>
84+
<_CopilotCliBinaryPath Condition="'$(_CopilotCliBinaryPath)' == ''">$(_CopilotCacheDir)\$(_CopilotBinary)</_CopilotCliBinaryPath>
6685
<_CopilotArchivePath>$(_CopilotCacheDir)\copilot.tgz</_CopilotArchivePath>
6786
<_CopilotNormalizedRegistryUrl>$([System.String]::Copy('$(CopilotNpmRegistryUrl)').TrimEnd('/'))</_CopilotNormalizedRegistryUrl>
6887
<_CopilotDownloadUrl>$(_CopilotNormalizedRegistryUrl)/@github/copilot-$(_CopilotPlatform)/-/copilot-$(_CopilotPlatform)-$(CopilotCliVersion).tgz</_CopilotDownloadUrl>
@@ -91,23 +110,29 @@
91110
<Error Condition="!Exists('$(_CopilotCliBinaryPath)')" Text="Failed to extract Copilot CLI binary to $(_CopilotCliBinaryPath)" />
92111
</Target>
93112

94-
<!-- Copy CLI binary to output runtimes folder and register for transitive copy -->
95-
<Target Name="_CopyCopilotCliToOutput" AfterTargets="Build" DependsOnTargets="_DownloadCopilotCli" Condition="'$(CopilotSkipCliDownload)' != 'true' And '$(_CopilotPlatform)' != ''">
113+
<!-- Copy CLI binary to output runtimes folder and register for transitive copy.
114+
Runs whenever we have a binary to place in the output: either the user provided
115+
CopilotCliBinaryPath, or the default download path is in effect. Skipped only
116+
when CopilotSkipCliDownload=true and no CopilotCliBinaryPath was supplied. -->
117+
<Target Name="_CopyCopilotCliToOutput" AfterTargets="Build" DependsOnTargets="_DownloadCopilotCli" Condition="('$(CopilotCliBinaryPath)' != '' Or '$(CopilotSkipCliDownload)' != 'true') And '$(_CopilotPlatform)' != ''">
96118
<PropertyGroup>
97-
<_CopilotCacheDir>$(IntermediateOutputPath)copilot-cli\$(CopilotCliVersion)\$(_CopilotPlatform)</_CopilotCacheDir>
98-
<_CopilotCliBinaryPath>$(_CopilotCacheDir)\$(_CopilotBinary)</_CopilotCliBinaryPath>
119+
<_CopilotCacheDir Condition="'$(_CopilotCacheDir)' == ''">$(IntermediateOutputPath)copilot-cli\$(CopilotCliVersion)\$(_CopilotPlatform)</_CopilotCacheDir>
120+
<_CopilotCliBinaryPath Condition="'$(_CopilotCliBinaryPath)' == ''">$(_CopilotCacheDir)\$(_CopilotBinary)</_CopilotCliBinaryPath>
99121
<_CopilotOutputDir>$(OutDir)runtimes\$(_CopilotRid)\native</_CopilotOutputDir>
100122
</PropertyGroup>
123+
<Error Condition="!Exists('$(_CopilotCliBinaryPath)')" Text="Copilot CLI binary not found at '$(_CopilotCliBinaryPath)'. Set CopilotCliBinaryPath to an existing CLI binary for the current RID, or remove CopilotSkipCliDownload=true to let the SDK download it." />
101124
<MakeDir Directories="$(_CopilotOutputDir)" />
102-
<Copy SourceFiles="$(_CopilotCliBinaryPath)" DestinationFolder="$(_CopilotOutputDir)" SkipUnchangedFiles="true" />
125+
<Copy SourceFiles="$(_CopilotCliBinaryPath)" DestinationFiles="$(_CopilotOutputDir)\$(_CopilotBinary)" SkipUnchangedFiles="true" />
103126
</Target>
104127

105-
<!-- Register CLI binary as content so it flows through project references -->
106-
<Target Name="_RegisterCopilotCliForCopy" BeforeTargets="GetCopyToOutputDirectoryItems" DependsOnTargets="_DownloadCopilotCli" Condition="'$(CopilotSkipCliDownload)' != 'true' And '$(_CopilotPlatform)' != ''">
128+
<!-- Register CLI binary as content so it flows through project references.
129+
Same gating semantics as _CopyCopilotCliToOutput. -->
130+
<Target Name="_RegisterCopilotCliForCopy" BeforeTargets="GetCopyToOutputDirectoryItems" DependsOnTargets="_DownloadCopilotCli" Condition="('$(CopilotCliBinaryPath)' != '' Or '$(CopilotSkipCliDownload)' != 'true') And '$(_CopilotPlatform)' != ''">
107131
<PropertyGroup>
108-
<_CopilotCacheDir>$(IntermediateOutputPath)copilot-cli\$(CopilotCliVersion)\$(_CopilotPlatform)</_CopilotCacheDir>
109-
<_CopilotCliBinaryPath>$(_CopilotCacheDir)\$(_CopilotBinary)</_CopilotCliBinaryPath>
132+
<_CopilotCacheDir Condition="'$(_CopilotCacheDir)' == ''">$(IntermediateOutputPath)copilot-cli\$(CopilotCliVersion)\$(_CopilotPlatform)</_CopilotCacheDir>
133+
<_CopilotCliBinaryPath Condition="'$(_CopilotCliBinaryPath)' == ''">$(_CopilotCacheDir)\$(_CopilotBinary)</_CopilotCliBinaryPath>
110134
</PropertyGroup>
135+
<Error Condition="!Exists('$(_CopilotCliBinaryPath)')" Text="Copilot CLI binary not found at '$(_CopilotCliBinaryPath)'. Set CopilotCliBinaryPath to an existing CLI binary for the current RID, or remove CopilotSkipCliDownload=true to let the SDK download it." />
111136
<ItemGroup>
112137
<ContentWithTargetPath Include="$(_CopilotCliBinaryPath)"
113138
TargetPath="runtimes\$(_CopilotRid)\native\$(_CopilotBinary)"
Lines changed: 299 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,299 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
*--------------------------------------------------------------------------------------------*/
4+
5+
using System.Diagnostics;
6+
using System.Runtime.CompilerServices;
7+
using System.Text;
8+
using Xunit;
9+
10+
namespace GitHub.Copilot.SDK.Test.Unit;
11+
12+
/// <summary>
13+
/// Integration tests for the MSBuild targets shipped in
14+
/// <c>dotnet/src/build/GitHub.Copilot.SDK.targets</c>. Each test creates a throwaway
15+
/// project that imports the targets file directly and invokes <c>dotnet build</c> in
16+
/// a subprocess so we exercise real MSBuild evaluation.
17+
/// </summary>
18+
/// <remarks>
19+
/// These tests deliberately do not exercise the network-bound default download path; they
20+
/// pin a fake <c>CopilotCliVersion</c> and supply a fake CLI binary via
21+
/// <c>CopilotCliBinaryPath</c>. That is sufficient to cover the regression in issue
22+
/// #921 ("preinstalled CLI is ignored and copy/register are skipped when
23+
/// CopilotSkipCliDownload=true").
24+
/// </remarks>
25+
public class MSBuildTargetsTests
26+
{
27+
private static readonly string TargetsFilePath = FindTargetsFile();
28+
29+
private static readonly string BinaryName = OperatingSystem.IsWindows() ? "copilot.exe" : "copilot";
30+
31+
[Fact]
32+
public async Task PreinstalledCliBinaryPath_IsHonored_DownloadSkipped_AndCopiedToOutput()
33+
{
34+
using var sandbox = MSBuildSandbox.Create();
35+
var preinstalled = sandbox.WritePreinstalledBinary("fake-cli-contents");
36+
37+
var result = await sandbox.BuildAsync(new Dictionary<string, string>
38+
{
39+
["CopilotCliBinaryPath"] = preinstalled,
40+
});
41+
42+
Assert.True(result.Succeeded, result.FailureMessage());
43+
44+
// Download message must be absent because the download target was skipped.
45+
Assert.DoesNotContain("Downloading Copilot CLI", result.StandardOutput, StringComparison.Ordinal);
46+
47+
// Binary must be placed at the canonical runtimes path so Client.cs can locate it.
48+
var outputPath = sandbox.ExpectedOutputBinary();
49+
Assert.True(File.Exists(outputPath), $"Expected CLI to be copied to '{outputPath}'.\n{result.FailureMessage()}");
50+
Assert.Equal(File.ReadAllText(preinstalled), File.ReadAllText(outputPath));
51+
}
52+
53+
[Fact]
54+
public async Task PreinstalledCliBinaryPath_NormalizesNonStandardFileNameToCanonical()
55+
{
56+
using var sandbox = MSBuildSandbox.Create();
57+
// Use an off-spec source filename to confirm the copy task renames it to copilot[.exe].
58+
var preinstalled = sandbox.WritePreinstalledBinary("custom-named", fileName: "my-copilot-binary-v1.bin");
59+
60+
var result = await sandbox.BuildAsync(new Dictionary<string, string>
61+
{
62+
["CopilotCliBinaryPath"] = preinstalled,
63+
});
64+
65+
Assert.True(result.Succeeded, result.FailureMessage());
66+
67+
var outputPath = sandbox.ExpectedOutputBinary();
68+
Assert.True(File.Exists(outputPath), $"Expected canonical binary at '{outputPath}'.\n{result.FailureMessage()}");
69+
}
70+
71+
[Fact]
72+
public async Task SkipCliDownload_WithoutBinaryPath_ProducesNoBinaryAndSucceeds()
73+
{
74+
using var sandbox = MSBuildSandbox.Create();
75+
76+
var result = await sandbox.BuildAsync(new Dictionary<string, string>
77+
{
78+
["CopilotSkipCliDownload"] = "true",
79+
});
80+
81+
Assert.True(result.Succeeded, result.FailureMessage());
82+
83+
// The runtimes folder may or may not be created by something else, but the binary
84+
// itself must not exist.
85+
Assert.False(File.Exists(sandbox.ExpectedOutputBinary()),
86+
$"Expected no CLI binary in output when CopilotSkipCliDownload=true and no path supplied.\n{result.FailureMessage()}");
87+
88+
// Download must also have been skipped.
89+
Assert.DoesNotContain("Downloading Copilot CLI", result.StandardOutput, StringComparison.Ordinal);
90+
}
91+
92+
[Fact]
93+
public async Task PreinstalledCliBinaryPath_WithSkipCliDownload_StillCopiesToOutput()
94+
{
95+
using var sandbox = MSBuildSandbox.Create();
96+
var preinstalled = sandbox.WritePreinstalledBinary("fake-cli-contents");
97+
98+
var result = await sandbox.BuildAsync(new Dictionary<string, string>
99+
{
100+
["CopilotCliBinaryPath"] = preinstalled,
101+
["CopilotSkipCliDownload"] = "true",
102+
});
103+
104+
Assert.True(result.Succeeded, result.FailureMessage());
105+
Assert.True(File.Exists(sandbox.ExpectedOutputBinary()), result.FailureMessage());
106+
}
107+
108+
[Fact]
109+
public async Task PreinstalledCliBinaryPath_NonExistentFile_FailsWithActionableError()
110+
{
111+
using var sandbox = MSBuildSandbox.Create();
112+
var nonexistent = Path.Combine(sandbox.ProjectDir, "does-not-exist", BinaryName);
113+
114+
var result = await sandbox.BuildAsync(new Dictionary<string, string>
115+
{
116+
["CopilotCliBinaryPath"] = nonexistent,
117+
});
118+
119+
Assert.False(result.Succeeded, "Build should have failed when CopilotCliBinaryPath points at a missing file.");
120+
Assert.Contains("Copilot CLI binary not found", result.StandardOutput, StringComparison.Ordinal);
121+
Assert.Contains(nonexistent, result.StandardOutput, StringComparison.Ordinal);
122+
}
123+
124+
private static string FindTargetsFile([CallerFilePath] string? thisFile = null)
125+
{
126+
// thisFile == <repo>/dotnet/test/Unit/MSBuildTargetsTests.cs
127+
if (thisFile is not null && File.Exists(thisFile))
128+
{
129+
var candidate = Path.GetFullPath(Path.Combine(
130+
Path.GetDirectoryName(thisFile)!, "..", "..", "src", "build", "GitHub.Copilot.SDK.targets"));
131+
if (File.Exists(candidate))
132+
{
133+
return candidate;
134+
}
135+
}
136+
137+
// Fall back to walking up from the test assembly location.
138+
var dir = AppContext.BaseDirectory;
139+
for (var i = 0; i < 8 && dir is not null; i++)
140+
{
141+
var candidate = Path.Combine(dir, "src", "build", "GitHub.Copilot.SDK.targets");
142+
if (File.Exists(candidate))
143+
{
144+
return candidate;
145+
}
146+
dir = Path.GetDirectoryName(dir);
147+
}
148+
149+
throw new InvalidOperationException(
150+
"Could not locate GitHub.Copilot.SDK.targets relative to test assembly or source file.");
151+
}
152+
153+
/// <summary>
154+
/// A throwaway directory containing a minimal csproj that imports the SDK targets
155+
/// file. Disposing removes the directory tree.
156+
/// </summary>
157+
private sealed class MSBuildSandbox : IDisposable
158+
{
159+
public string ProjectDir { get; }
160+
161+
private MSBuildSandbox(string projectDir)
162+
{
163+
ProjectDir = projectDir;
164+
}
165+
166+
public static MSBuildSandbox Create()
167+
{
168+
var dir = Path.Combine(Path.GetTempPath(), "copilot-sdk-targets-test-" + Guid.NewGuid().ToString("N"));
169+
Directory.CreateDirectory(dir);
170+
171+
// Minimal class library that imports the SDK targets with a pinned fake
172+
// CopilotCliVersion so the targets do not need the generated props file.
173+
var csproj = $"""
174+
<Project Sdk="Microsoft.NET.Sdk">
175+
<PropertyGroup>
176+
<TargetFramework>net8.0</TargetFramework>
177+
<CopilotCliVersion>0.0.0-test</CopilotCliVersion>
178+
<EnableDefaultCompileItems>true</EnableDefaultCompileItems>
179+
</PropertyGroup>
180+
<Import Project="{TargetsFilePath}" />
181+
</Project>
182+
""";
183+
File.WriteAllText(Path.Combine(dir, "App.csproj"), csproj);
184+
File.WriteAllText(Path.Combine(dir, "Stub.cs"), "namespace CopilotSdkTargetsTest { internal static class Stub { } }\n");
185+
186+
return new MSBuildSandbox(dir);
187+
}
188+
189+
public string WritePreinstalledBinary(string contents, string? fileName = null)
190+
{
191+
var preinstallDir = Path.Combine(ProjectDir, "preinstall");
192+
Directory.CreateDirectory(preinstallDir);
193+
// Strip any path information from fileName so it cannot escape preinstallDir.
194+
var safeFileName = string.IsNullOrEmpty(fileName) ? BinaryName : Path.GetFileName(fileName);
195+
var path = Path.Combine(preinstallDir, safeFileName);
196+
File.WriteAllText(path, contents);
197+
return path;
198+
}
199+
200+
public string ExpectedOutputBinary()
201+
{
202+
var rid = GetPortableRid();
203+
return Path.Combine(ProjectDir, "bin", "Debug", "net8.0", "runtimes", rid, "native", BinaryName);
204+
}
205+
206+
public async Task<BuildResult> BuildAsync(IDictionary<string, string> properties)
207+
{
208+
var args = new StringBuilder("build --nologo -clp:NoSummary");
209+
foreach (var (key, value) in properties)
210+
{
211+
// Quote the value so paths with spaces are preserved.
212+
args.Append(" /p:").Append(key).Append('=').Append('"').Append(value).Append('"');
213+
}
214+
215+
var psi = new ProcessStartInfo("dotnet", args.ToString())
216+
{
217+
WorkingDirectory = ProjectDir,
218+
RedirectStandardOutput = true,
219+
RedirectStandardError = true,
220+
UseShellExecute = false,
221+
CreateNoWindow = true,
222+
};
223+
// Avoid inheriting the parent's MSBuildSDKsPath/RuntimeIdentifier from the
224+
// running test host; the subprocess should resolve its own SDK and pick the
225+
// RID that matches ExpectedOutputBinary().
226+
psi.Environment.Remove("MSBuildSDKsPath");
227+
psi.Environment.Remove("RuntimeIdentifier");
228+
229+
using var process = Process.Start(psi) ?? throw new InvalidOperationException("Failed to start dotnet build subprocess.");
230+
231+
// Drain both streams concurrently to avoid deadlocks on full pipe buffers.
232+
var stdoutTask = process.StandardOutput.ReadToEndAsync();
233+
var stderrTask = process.StandardError.ReadToEndAsync();
234+
235+
// Generous timeout: dotnet restore + build of an empty project on a slow CI
236+
// worker can take ~60s the first time. We keep individual tests short by
237+
// using minimal projects.
238+
using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(5));
239+
try
240+
{
241+
await process.WaitForExitAsync(cts.Token);
242+
}
243+
catch (OperationCanceledException)
244+
{
245+
try { process.Kill(entireProcessTree: true); }
246+
catch (InvalidOperationException) { /* process already exited */ }
247+
catch (NotSupportedException) { /* not supported on this platform */ }
248+
catch (System.ComponentModel.Win32Exception) { /* kill failed; best effort */ }
249+
throw new TimeoutException($"dotnet build did not complete within the timeout for args: {args}");
250+
}
251+
252+
return new BuildResult(
253+
ExitCode: process.ExitCode,
254+
StandardOutput: await stdoutTask,
255+
StandardError: await stderrTask,
256+
CommandLine: $"dotnet {args}");
257+
}
258+
259+
public void Dispose()
260+
{
261+
try { Directory.Delete(ProjectDir, recursive: true); }
262+
catch (IOException) { /* cleanup is best effort */ }
263+
catch (UnauthorizedAccessException) { /* cleanup is best effort */ }
264+
}
265+
266+
private static string GetPortableRid()
267+
{
268+
if (OperatingSystem.IsWindows())
269+
{
270+
return System.Runtime.InteropServices.RuntimeInformation.OSArchitecture switch
271+
{
272+
System.Runtime.InteropServices.Architecture.Arm64 => "win-arm64",
273+
_ => "win-x64",
274+
};
275+
}
276+
if (OperatingSystem.IsMacOS())
277+
{
278+
return System.Runtime.InteropServices.RuntimeInformation.OSArchitecture switch
279+
{
280+
System.Runtime.InteropServices.Architecture.Arm64 => "osx-arm64",
281+
_ => "osx-x64",
282+
};
283+
}
284+
return System.Runtime.InteropServices.RuntimeInformation.OSArchitecture switch
285+
{
286+
System.Runtime.InteropServices.Architecture.Arm64 => "linux-arm64",
287+
_ => "linux-x64",
288+
};
289+
}
290+
}
291+
292+
private sealed record BuildResult(int ExitCode, string StandardOutput, string StandardError, string CommandLine)
293+
{
294+
public bool Succeeded => ExitCode == 0;
295+
296+
public string FailureMessage() =>
297+
$"{CommandLine}\nExitCode: {ExitCode}\n--- STDOUT ---\n{StandardOutput}\n--- STDERR ---\n{StandardError}";
298+
}
299+
}

0 commit comments

Comments
 (0)