Skip to content

Commit 50c9076

Browse files
GregorBiswangerGregorBiswanger
authored
fix: replace ReadAllLines with ReadAllText in MigrationChecks.targets… (#1062)
* fix: replace ReadAllLines with ReadAllText in MigrationChecks.targets (fixes #1035) System.IO.File::ReadAllLines is not available as an MSBuild property function on all platforms (e.g. macOS GitHub Actions), causing MSB4185. ReadAllText is universally supported by MSBuild and sufficient for the regex check that detects 'electron' references in a root package.json (ELECTRON008). The intermediate ItemGroup for line accumulation is no longer needed. * test: strengthen migration checks test with exit code assertion and remove reserved MSBuildProjectDirectory override * test: locate MigrationChecks.targets via directory walk for path robustness * test: replace em-dashes with ASCII hyphens to avoid bidi/hidden Unicode warning * test: split build test into clean and electron-containing package.json scenarios
1 parent d7e1a09 commit 50c9076

2 files changed

Lines changed: 197 additions & 5 deletions

File tree

‎src/ElectronNET.IntegrationTests/Tests/MigrationChecksTargetsTests.cs‎

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
using System.Diagnostics;
2+
3+
namespace ElectronNET.IntegrationTests.Tests;
4+
5+
/// <summary>
6+
/// Unit tests for ElectronNET.MigrationChecks.targets - no Electron runtime required.
7+
/// Covers GitHub issue #1035: System.IO.File.ReadAllLines is not available as an MSBuild
8+
/// property function on all platforms (e.g. macOS GitHub Actions), causing MSB4185.
9+
/// </summary>
10+
public class MigrationChecksTargetsTests
11+
{
12+
private static readonly string TargetsFilePath = FindTargetsFile();
13+
14+
/// <summary>
15+
/// Walks up the directory tree from <see cref="AppContext.BaseDirectory"/> until it finds
16+
/// the migration checks targets file. This is robust against varying output paths
17+
/// (with or without RID subfolder, debug/release, etc.).
18+
/// </summary>
19+
private static string FindTargetsFile()
20+
{
21+
const string RelativeFromRepoRoot =
22+
"src/ElectronNET/build/ElectronNET.MigrationChecks.targets";
23+
const string RelativeFromSrc =
24+
"ElectronNET/build/ElectronNET.MigrationChecks.targets";
25+
26+
var dir = new DirectoryInfo(AppContext.BaseDirectory);
27+
while (dir != null)
28+
{
29+
var fromRepoRoot = Path.Combine(dir.FullName, RelativeFromRepoRoot);
30+
if (File.Exists(fromRepoRoot))
31+
{
32+
return Path.GetFullPath(fromRepoRoot);
33+
}
34+
35+
var fromSrc = Path.Combine(dir.FullName, RelativeFromSrc);
36+
if (File.Exists(fromSrc))
37+
{
38+
return Path.GetFullPath(fromSrc);
39+
}
40+
41+
dir = dir.Parent;
42+
}
43+
44+
throw new FileNotFoundException(
45+
"Could not locate ElectronNET.MigrationChecks.targets by walking up from " +
46+
$"'{AppContext.BaseDirectory}'.");
47+
}
48+
49+
// -----------------------------------------------------------------------
50+
// Content-level test (RED before fix, GREEN after fix on ALL platforms)
51+
// -----------------------------------------------------------------------
52+
53+
[Fact]
54+
public void MigrationChecksTargets_ShouldNotUseReadAllLines()
55+
{
56+
// The file must exist - if this fails the path constant above is wrong.
57+
File.Exists(TargetsFilePath).Should().BeTrue(
58+
$"targets file must exist at '{TargetsFilePath}'");
59+
60+
var content = File.ReadAllText(TargetsFilePath);
61+
62+
// System.IO.File::ReadAllLines is not in the MSBuild property-function
63+
// whitelist on all platforms (MSB4185 on macOS GitHub Actions, see #1035).
64+
// ReadAllText must be used instead.
65+
content.Should().NotContain(
66+
"::ReadAllLines(",
67+
"because ReadAllLines is not available as an MSBuild property function on all " +
68+
"platforms. Use ReadAllText instead (GitHub issue #1035).");
69+
}
70+
71+
// -----------------------------------------------------------------------
72+
// Functional build test - verifies no MSB4185 at runtime
73+
// (RED on platforms where ReadAllLines is restricted, GREEN after fix)
74+
// -----------------------------------------------------------------------
75+
76+
[Fact]
77+
public async Task MigrationChecksTargets_BuildWithCleanPackageJson_ShouldSucceedWithoutMSB4185()
78+
{
79+
// Positive case: a package.json that does NOT mention electron.
80+
// The migration check must successfully read the file via ReadAllText
81+
// (the code path fixed by issue #1035) without producing MSB4185.
82+
83+
var tempDir = CreateTempProjectDirectory();
84+
try
85+
{
86+
await File.WriteAllTextAsync(
87+
Path.Combine(tempDir, "package.json"),
88+
"""{ "devDependencies": { "vite": "^5.0.0" } }""");
89+
90+
await WriteMinimalCsprojAsync(tempDir);
91+
92+
var (exitCode, output) = await RunDotnetBuildAsync(tempDir);
93+
94+
exitCode.Should().Be(0,
95+
$"the build must succeed when the package.json contains no electron references. " +
96+
$"Full build output:\n{output}");
97+
98+
output.Should().NotContain(
99+
"MSB4185",
100+
$"ReadAllLines must not be used as an MSBuild property function. " +
101+
$"Full build output:\n{output}");
102+
}
103+
finally
104+
{
105+
Directory.Delete(tempDir, recursive: true);
106+
}
107+
}
108+
109+
[Fact]
110+
public async Task MigrationChecksTargets_BuildWithPackageJsonContainingElectron_ShouldEmitELECTRON008WarningWithoutMSB4185()
111+
{
112+
// Negative case: a package.json that DOES contain "electron".
113+
// The migration check must still read the file successfully (no MSB4185)
114+
// and must emit the expected ELECTRON008 warning. ELECTRON008 is a
115+
// <Warning>, not an <Error>, so the build itself still succeeds.
116+
117+
var tempDir = CreateTempProjectDirectory();
118+
try
119+
{
120+
await File.WriteAllTextAsync(
121+
Path.Combine(tempDir, "package.json"),
122+
"""{ "devDependencies": { "electron": "^30.0.0" } }""");
123+
124+
await WriteMinimalCsprojAsync(tempDir);
125+
126+
var (exitCode, output) = await RunDotnetBuildAsync(tempDir);
127+
128+
exitCode.Should().Be(0,
129+
$"ELECTRON008 is a Warning (not an Error) so the build itself must still " +
130+
$"succeed. Full build output:\n{output}");
131+
132+
output.Should().NotContain(
133+
"MSB4185",
134+
$"ReadAllLines must not be used as an MSBuild property function. " +
135+
$"Full build output:\n{output}");
136+
137+
output.Should().Contain(
138+
"ELECTRON008",
139+
$"the migration check must still detect electron references in package.json " +
140+
$"after the ReadAllText migration. Full build output:\n{output}");
141+
}
142+
finally
143+
{
144+
Directory.Delete(tempDir, recursive: true);
145+
}
146+
}
147+
148+
// -----------------------------------------------------------------------
149+
// Helpers
150+
// -----------------------------------------------------------------------
151+
152+
private static string CreateTempProjectDirectory()
153+
{
154+
var tempDir = Path.Combine(Path.GetTempPath(), $"electron-net-test-{Guid.NewGuid():N}");
155+
Directory.CreateDirectory(tempDir);
156+
return tempDir;
157+
}
158+
159+
private static Task WriteMinimalCsprojAsync(string tempDir)
160+
{
161+
// A minimal csproj that only imports the migration checks targets to keep the
162+
// build fast. Note: MSBuildProjectDirectory is a reserved MSBuild property and
163+
// must not be redefined manually; MSBuild sets it automatically to the folder
164+
// of the csproj (which is tempDir here).
165+
var targetsPathEscaped = TargetsFilePath.Replace("'", "&apos;");
166+
return File.WriteAllTextAsync(
167+
Path.Combine(tempDir, "TestApp.csproj"),
168+
$"""
169+
<Project>
170+
<Import Project="{targetsPathEscaped}" />
171+
<Target Name="Build" DependsOnTargets="ElectronMigrationChecks" />
172+
</Project>
173+
""");
174+
}
175+
176+
private static async Task<(int ExitCode, string Output)> RunDotnetBuildAsync(string workingDirectory)
177+
{
178+
var psi = new ProcessStartInfo("dotnet", "build --nologo -v:minimal")
179+
{
180+
WorkingDirectory = workingDirectory,
181+
RedirectStandardOutput = true,
182+
RedirectStandardError = true,
183+
UseShellExecute = false,
184+
};
185+
186+
using var process = Process.Start(psi)!;
187+
var stdOut = await process.StandardOutput.ReadToEndAsync();
188+
var stdErr = await process.StandardError.ReadToEndAsync();
189+
await process.WaitForExitAsync();
190+
191+
return (process.ExitCode, stdOut + stdErr);
192+
}
193+
}

‎src/ElectronNET/build/ElectronNET.MigrationChecks.targets‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -73,12 +73,11 @@ EXCEPTION:
7373
<Target Name="ElectronCheckRootPackageJsonNoElectron"
7474
Condition="Exists('$(MSBuildProjectDirectory)\package.json')">
7575

76-
<ItemGroup>
77-
<_RootPackageJsonLines Include="$([System.IO.File]::ReadAllLines('$(MSBuildProjectDirectory)\package.json'))" />
78-
</ItemGroup>
79-
8076
<PropertyGroup>
81-
<_RootPackageJsonContent>@(_RootPackageJsonLines, ' ')</_RootPackageJsonContent>
77+
<!-- ReadAllText is used here instead of ReadAllLines because ReadAllLines is not available
78+
as an MSBuild property function on all platforms (e.g. macOS GitHub Actions), which
79+
caused MSB4185. ReadAllText is universally supported. See GitHub issue #1035. -->
80+
<_RootPackageJsonContent>$([System.IO.File]::ReadAllText('$(MSBuildProjectDirectory)\package.json'))</_RootPackageJsonContent>
8281
<_RootPackageJsonHasElectron>false</_RootPackageJsonHasElectron>
8382
<_RootPackageJsonHasElectron Condition="$([System.Text.RegularExpressions.Regex]::IsMatch('$(_RootPackageJsonContent)', 'electron', System.Text.RegularExpressions.RegexOptions.IgnoreCase))">true</_RootPackageJsonHasElectron>
8483
</PropertyGroup>

0 commit comments

Comments
 (0)