-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Migrate manifest-handling tasks to TaskEnvironment API #13177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
JanProvaznik
merged 22 commits into
dotnet:main
from
JanProvaznik:resource-manifests-migration
Apr 30, 2026
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
e5763fd
Migrate manifest-handling tasks to TaskEnvironment API
JanProvaznik 96ef4a4
Add path canonicalization and TaskEnvironment tests
JanProvaznik 5d24c3b
Merge branch 'main' into resource-manifests-migration
JanProvaznik b2876ad
Fix compatibility issues: preserve original paths in error messages a…
JanProvaznik f6d9071
Address PR review: AbsolutePath types, GetCanonicalForm, remove unuse…
JanProvaznik 6d3a74b
Deduplicate skill file: merge Sin 5+7, unify checklists, trim redunda…
JanProvaznik 86f7785
Remove skill file changes (extracted to separate PR)
JanProvaznik ccbec40
Merge remote-tracking branch 'origin/main' into resource-manifests-mi…
JanProvaznik 93cbd5e
Fix Constants ambiguity after merge with main
JanProvaznik d92d315
Remove [MSBuildMultiThreadableTask] from abstract classes (Inherited=…
JanProvaznik 1cd9eb3
Fix CWD dependency in Util.SortItems: use TaskEnvironment for path ca…
JanProvaznik 651ea49
Change CreateFileStream delegate to take AbsolutePath instead of string
JanProvaznik 80bd870
Merge branch 'main' into resource-manifests-migration
JanProvaznik c8b44ad
Merge branch 'main' into resource-manifests-migration
JanProvaznik 7b75e5a
Merge branch 'main' into resource-manifests-migration
JanProvaznik a8525b7
fallback behavior
JanProvaznik c98970d
remove obsolete test setup
JanProvaznik a8b1981
note the behavior of Manifest.ResolveFiles for consumers
JanProvaznik a697593
absolutize before resolving
JanProvaznik d383811
fix test absolutepath handling
JanProvaznik 9795b84
fix test pr comments
JanProvaznik a2e90df
Merge branch 'main' into resource-manifests-migration
JanProvaznik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,232 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.IO; | ||
| using Microsoft.Build.Framework; | ||
| using Microsoft.Build.Tasks; | ||
| using Microsoft.Build.UnitTests; | ||
| using Microsoft.Build.Utilities; | ||
| using Shouldly; | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.Build.Tasks.UnitTests | ||
| { | ||
| /// <summary> | ||
| /// Tests verifying TaskEnvironment migration compatibility for manifest tasks. | ||
| /// These tests focus on path handling changes from the migration. | ||
| /// </summary> | ||
| public class ManifestTaskEnvironmentTests | ||
| { | ||
| private readonly ITestOutputHelper _output; | ||
|
|
||
| public ManifestTaskEnvironmentTests(ITestOutputHelper output) => _output = output; | ||
| // Test 1: Empty ItemSpec - verifies exception handling matches pre-migration behavior | ||
| // GetAbsolutePath throws on empty, but this flows through existing exception handling | ||
| [Fact] | ||
| public void CreateManifestResourceName_EmptyItemSpec_ShouldFail() | ||
| { | ||
| var engine = new MockEngine(_output); | ||
| var task = new CreateCSharpManifestResourceName | ||
| { | ||
| BuildEngine = engine, | ||
| ResourceFiles = new ITaskItem[] { new TaskItem("") }, | ||
| RootNamespace = "Test" | ||
| }; | ||
|
|
||
| // On .NET Framework: returns false with logged error | ||
| // On .NET Core+: throws ArgumentNullException (pre-existing behavior in Path.GetDirectoryName) | ||
| #if NETFRAMEWORK | ||
| bool result = task.Execute(); | ||
| result.ShouldBeFalse(); | ||
| #else | ||
| Should.Throw<ArgumentNullException>(() => task.Execute()); | ||
| #endif | ||
| } | ||
|
|
||
| // Test 2: Path with .. segments - critical test for canonicalization | ||
| // GetAbsolutePath does NOT canonicalize, so we wrap with Path.GetFullPath where needed | ||
| [Fact] | ||
| public void CreateManifestResourceName_PathWithDotDot_ShouldResolve() | ||
| { | ||
| using var env = TestEnvironment.Create(_output); | ||
| var folder = env.CreateFolder(); | ||
| var subFolder = Path.Combine(folder.Path, "sub"); | ||
| Directory.CreateDirectory(subFolder); | ||
|
|
||
| var resxPath = Path.Combine(subFolder, "Test.resx"); | ||
| File.WriteAllText(resxPath, "<root></root>"); | ||
|
|
||
| var csPath = Path.Combine(subFolder, "Test.cs"); | ||
| File.WriteAllText(csPath, "namespace Test { class Test { } }"); | ||
|
|
||
| // Use path with .. segments - tests canonicalization | ||
| var pathWithDotDot = Path.Combine(folder.Path, "sub", "..", "sub", "Test.resx"); | ||
|
|
||
| var task = new CreateCSharpManifestResourceName | ||
| { | ||
| BuildEngine = new MockEngine(_output), | ||
| ResourceFiles = new ITaskItem[] { new TaskItem(pathWithDotDot) }, | ||
| RootNamespace = "Test", | ||
| UseDependentUponConvention = true | ||
| }; | ||
|
|
||
| bool result = task.Execute(); | ||
| result.ShouldBeTrue(); | ||
| } | ||
|
|
||
| // Test 3: Forward slashes - tests path normalization | ||
| [Fact] | ||
| public void CreateManifestResourceName_ForwardSlashes_ShouldWork() | ||
| { | ||
| using var env = TestEnvironment.Create(_output); | ||
| var folder = env.CreateFolder(); | ||
| var subFolder = Path.Combine(folder.Path, "Resources"); | ||
| Directory.CreateDirectory(subFolder); | ||
|
|
||
| var resxPath = Path.Combine(subFolder, "Strings.resx"); | ||
| File.WriteAllText(resxPath, "<root></root>"); | ||
|
|
||
| // Replace backslashes with forward slashes | ||
| var pathWithForwardSlashes = resxPath.Replace('\\', '/'); | ||
|
|
||
| var task = new CreateCSharpManifestResourceName | ||
| { | ||
| BuildEngine = new MockEngine(_output), | ||
| ResourceFiles = new ITaskItem[] { new TaskItem(pathWithForwardSlashes) }, | ||
| RootNamespace = "Test" | ||
| }; | ||
|
|
||
| bool result = task.Execute(); | ||
| result.ShouldBeTrue(); | ||
| } | ||
|
|
||
| // Test 4: Mixed slashes - tests path normalization handles both | ||
| [Fact] | ||
| public void CreateManifestResourceName_MixedSlashes_ShouldWork() | ||
| { | ||
| using var env = TestEnvironment.Create(_output); | ||
| var folder = env.CreateFolder(); | ||
| var subFolder = Path.Combine(folder.Path, "Sub", "Folder"); | ||
| Directory.CreateDirectory(subFolder); | ||
|
|
||
| var resxPath = Path.Combine(subFolder, "Test.resx"); | ||
| File.WriteAllText(resxPath, "<root></root>"); | ||
|
|
||
| // Mix forward and back slashes | ||
| var mixedPath = folder.Path + "/Sub\\Folder/Test.resx"; | ||
|
|
||
| var task = new CreateCSharpManifestResourceName | ||
| { | ||
| BuildEngine = new MockEngine(_output), | ||
| ResourceFiles = new ITaskItem[] { new TaskItem(mixedPath) }, | ||
| RootNamespace = "Test" | ||
| }; | ||
|
|
||
| bool result = task.Execute(); | ||
| result.ShouldBeTrue(); | ||
| } | ||
|
|
||
| // Test 5: AddToWin32Manifest with null ApplicationManifest - tests graceful handling | ||
| [Fact] | ||
| public void AddToWin32Manifest_NullApplicationManifest_HandledGracefully() | ||
| { | ||
| using var env = TestEnvironment.Create(_output); | ||
| var folder = env.CreateFolder(); | ||
|
|
||
| var task = new AddToWin32Manifest | ||
| { | ||
| BuildEngine = new MockEngine(_output), | ||
| ApplicationManifest = null, | ||
| OutputDirectory = folder.Path, | ||
| SupportedArchitectures = "amd64" | ||
| }; | ||
|
|
||
| // Null is treated as "no manifest" - should generate new one | ||
| bool result = task.Execute(); | ||
| result.ShouldBeTrue(); | ||
| } | ||
|
|
||
| // Test 6: Batch processing - one error should not abort remaining items | ||
| [Fact] | ||
| public void CreateManifestResourceName_BatchProcessing_ContinuesAfterError() | ||
| { | ||
| using var env = TestEnvironment.Create(_output); | ||
| var folder = env.CreateFolder(); | ||
|
|
||
| // Create one valid resource | ||
| var validPath = Path.Combine(folder.Path, "Valid.resx"); | ||
| File.WriteAllText(validPath, "<root></root>"); | ||
|
|
||
| // Create another valid resource | ||
| var valid2Path = Path.Combine(folder.Path, "Valid2.resx"); | ||
| File.WriteAllText(valid2Path, "<root></root>"); | ||
|
|
||
| // Invalid: DependentUpon points to non-existent file | ||
| var invalidItem = new TaskItem(validPath); | ||
| invalidItem.SetMetadata("DependentUpon", "NonExistent.cs"); | ||
|
|
||
| var validItem = new TaskItem(valid2Path); | ||
|
|
||
| var task = new CreateCSharpManifestResourceName | ||
| { | ||
| BuildEngine = new MockEngine(_output), | ||
| ResourceFiles = new ITaskItem[] { invalidItem, validItem }, | ||
| RootNamespace = "Test" | ||
| }; | ||
|
|
||
| // Should return false due to error, but should still process both items | ||
| bool result = task.Execute(); | ||
| result.ShouldBeFalse(); | ||
| // The valid item should still have been processed successfully | ||
| task.ManifestResourceNames[1].ShouldNotBeNull(); | ||
| task.ManifestResourceNames[1].ItemSpec.ShouldNotBeNullOrEmpty(); | ||
| } | ||
|
|
||
| // Test 7: Deeply nested folder - tests path handling with many segments | ||
| [Fact] | ||
| public void CreateManifestResourceName_DeepNesting_ShouldWork() | ||
| { | ||
| using var env = TestEnvironment.Create(_output); | ||
| var folder = env.CreateFolder(); | ||
| var deepFolder = Path.Combine(folder.Path, "a", "b", "c", "d", "e"); | ||
| Directory.CreateDirectory(deepFolder); | ||
|
|
||
| var resxPath = Path.Combine(deepFolder, "Test.resx"); | ||
| File.WriteAllText(resxPath, "<root></root>"); | ||
|
|
||
| var task = new CreateCSharpManifestResourceName | ||
| { | ||
| BuildEngine = new MockEngine(_output), | ||
| ResourceFiles = new ITaskItem[] { new TaskItem(resxPath) }, | ||
| RootNamespace = "Test" | ||
| }; | ||
|
|
||
| bool result = task.Execute(); | ||
| result.ShouldBeTrue(); | ||
| } | ||
|
|
||
| // Test 8: Path with spaces - tests no issues with space handling | ||
| [Fact] | ||
| public void CreateManifestResourceName_PathWithSpaces_ShouldWork() | ||
| { | ||
| using var env = TestEnvironment.Create(_output); | ||
| var folder = env.CreateFolder(); | ||
| var spaceFolder = Path.Combine(folder.Path, "My Resources"); | ||
| Directory.CreateDirectory(spaceFolder); | ||
|
|
||
| var resxPath = Path.Combine(spaceFolder, "My Strings.resx"); | ||
| File.WriteAllText(resxPath, "<root></root>"); | ||
|
|
||
| var task = new CreateCSharpManifestResourceName | ||
| { | ||
| BuildEngine = new MockEngine(_output), | ||
| ResourceFiles = new ITaskItem[] { new TaskItem(resxPath) }, | ||
| RootNamespace = "Test" | ||
| }; | ||
|
|
||
| bool result = task.Execute(); | ||
| result.ShouldBeTrue(); | ||
| } | ||
| } | ||
| } | ||
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
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.