Enlighten RequiresFramework35SP1Assembly task for multithreaded mode

[jankratochvilcz]

Enlighten RequiresFramework35SP1Assembly task for multithreaded mode

Parent: #11834

Context

RequiresFramework35SP1Assembly is one of the remaining tasks listed in the migration epic (#11834) under "Other (either simple or with unknown issues)". It computes a single boolean output (RequiresMinimumFramework35SP1) from a handful of inputs that describe a ClickOnce publish: an error-report URL string, a target-framework version string, a CreateDesktopShortcut bool, a SigningManifests bool, several ITaskItem[] collections (ReferencedAssemblies, Assemblies, Files), two single ITaskItem references (DeploymentManifestEntryPoint, EntryPoint), and a SuiteName string.

Tracing every code path in src/Tasks/RequiresFramework35SP1Assembly.cs:

Execute
  ├─ HasErrorUrl              → string.IsNullOrEmpty(ErrorReportUrl)
  ├─ HasCreatedShortcut       → CreateDesktopShortcut getter
  │     └─ CompareFrameworkVersions → Version.Parse / Version.CompareTo
  ├─ UncheckedSigning         → !SigningManifests
  ├─ ExcludeReferenceFromHashing
  │     └─ HasExcludedFileOrSP1File / IsExcludedFileOrSP1File
  │           └─ ITaskItem.GetMetadata + string.Equals(OrdinalIgnoreCase)
  └─ HasSuiteName             → string.IsNullOrEmpty(SuiteName)

Nothing in this graph touches:

• the file system (File.*, Directory.*, FileInfo, Path.GetFullPath, Path.Combine)
• the working directory (Environment.CurrentDirectory, Directory.GetCurrentDirectory)
• environment variables (Environment.GetEnvironmentVariable)
• process start (ProcessStartInfo, Process.Start)
• AppDomain or Assembly.Load*
• any shared mutable static state

The task is purely in-memory — string comparisons, Version parsing/compare, and ITaskItem metadata reads. There is no cwd-dependence buried in helpers (Sin 7): Constants.NET35SP1AssemblyIdentity / Constants.NET35ClientAssemblyIdentity are static string arrays, and ConvertFrameworkVersionToString only calls Version.Parse. This is the simplest possible migration in the epic.

The task is referenced from src/Tasks/Microsoft.Common.CurrentVersion.targets (line 4347) in the ClickOnce publish flow — it sets the _DeploymentRequiresMinimumFramework35SP1 property used downstream. End-to-end exercise of that flow lives outside this repo.

Approach

Apply the PR #13045 pattern (attribute-only baseline): the task qualifies for multithreaded mode without any code change beyond the attribute.

1. Decorate RequiresFramework35SP1Assembly with [MSBuildMultiThreadableTask].
2. Do not implement IMultiThreadableTask — there is no TaskEnvironment consumer (no path absolutization, no env vars, no process start). Per the SKILL Step 1b, the attribute alone is sufficient.
3. No signature changes, no helper modifications, no behavioral change.

Why no ChangeWave

There is no behavioral change at all. The body of Execute is unchanged; output is byte-identical for every input on both legacy and multithreaded execution. ChangeWave is unnecessary.

Test coverage assessment

Existing direct unit tests

None. A search of src/Tasks.UnitTests for RequiresFramework35SP1Assembly returns no files. The task ships untested at the unit level.

Integration tests in this repo

None. Only consumed by Microsoft.Common.CurrentVersion.targets in the ClickOnce/publish flow. End-to-end coverage lives in dotnet/sdk and the VS repo, not here. We will not add E2E publish tests in this repo as part of this issue.

Gaps to fill in this PR

Because the task currently has zero direct coverage, this migration is a good opportunity to add a minimum baseline. Even though no behavior changes, tests are needed to lock in the contract before the next migration touches it.

• G1 — All-defaults / all-empty test. No properties set → RequiresMinimumFramework35SP1 == false, Execute() returns true.
• G2 — Each predicate triggers independently. Five tests, one per disjunct in Execute: ErrorReportUrl set; CreateDesktopShortcut == true with TargetFrameworkVersion ≥ v3.5; SigningManifests == false; an ITaskItem carrying IncludeHash=false in each of ReferencedAssemblies / Assemblies / Files / DeploymentManifestEntryPoint / EntryPoint; SuiteName set. Each should set RequiresMinimumFramework35SP1 == true.
• G3 — CreateDesktopShortcut framework-version gate. CreateDesktopShortcut == true with TargetFrameworkVersion = "v2.0" → still false (suppressed by CompareFrameworkVersions < 0). Same with "v3.5" → true. Also covers the "v"-prefix branch in ConvertFrameworkVersionToString.
• G4 — SP1-identity short-circuit. An ITaskItem whose ItemSpec matches Constants.NET35SP1AssemblyIdentity[0] or Constants.NET35ClientAssemblyIdentity[0] triggers RequiresMinimumFramework35SP1 == true even when IncludeHash is unset.
• G5 — Concurrency smoke test. Two task instances built in parallel with different inputs produce independent results. (SKILL red-team Phase 4. Trivially passes for a stateless task, but documents the MT contract.)

No TaskEnvironment injection is required in tests because the task does not implement IMultiThreadableTask.

Acceptance criteria

• RequiresFramework35SP1Assembly decorated [MSBuildMultiThreadableTask].
• IMultiThreadableTask is not implemented (no TaskEnvironment consumer in the task).
• No code changes to Execute or any helper method (HasErrorUrl, HasCreatedShortcut, UncheckedSigning, ExcludeReferenceFromHashing, HasExcludedFileOrSP1File, IsExcludedFileOrSP1File, HasSuiteName, CompareFrameworkVersions, ConvertFrameworkVersionToString).
• No changes to Microsoft.Common.CurrentVersion.targets invocation site.
• New file src/Tasks.UnitTests/RequiresFramework35SP1Assembly_Tests.cs added with tests G1–G5.
• All new tests pass on net472 and net10.0.
• RequiresMinimumFramework35SP1 output is byte-identical to pre-migration behavior across the full G2/G3/G4 input matrix.
• No ChangeWave required.
• .\build.cmd -v quiet succeeds; Tasks.UnitTests passes.
• No new compiler warnings (warnings-as-errors in official build).
• multithreaded-task-migration SKILL sign-off checklist walked and passes (most items are N/A for an attribute-only migration; document N/A explicitly in the PR description).

Implementation order

1. Add [MSBuildMultiThreadableTask] to RequiresFramework35SP1Assembly.
2. Create src/Tasks.UnitTests/RequiresFramework35SP1Assembly_Tests.cs with tests G1–G5, using MockEngine and Shouldly per the testing instructions.
3. Run dotnet test src/Tasks.UnitTests/Microsoft.Build.Tasks.UnitTests.csproj --filter RequiresFramework35SP1Assembly and confirm green on both target frameworks.
4. Run .\build.cmd -v quiet to verify clean build with no new warnings.
5. Walk the SKILL sign-off checklist; mark N/A items with a one-line justification in the PR.

Risks / open questions

• ClickOnce publish flow under MT is not exercised by this repo's tests. Coordination with dotnet/sdk is recommended before MT mode ships generally, but is not blocking for this PR — the task is provably stateless and side-effect-free in isolation.
• None of the SKILL's "7 Deadly Sins" apply: no [Output] path strings (Sin 1), no path-formatted error messages (Sin 2), no ?? additions (Sin 3), no try/catch around GetAbsolutePath (Sin 4), no path dictionary keys (Sin 5), no GetAbsolutePath calls at all (Sin 6), no helpers reaching cwd-dependent APIs (Sin 7).

References

• Parent epic: Migrate Tasks in the MSbuild repo to the new Task type #11834
• Pattern PRs:
  • Enlighten more tasks that require no change #13045 — Enlighten more tasks that require no change (attribute-only baseline; direct precedent for this issue)
  • Enlighten public versions of intrinsic tasks. #13267 — Enlighten public versions of intrinsic tasks (absolutize-at-boundary pattern; for reference)
  • Enlighten GetFrameworkPath and GetFrameworkSdkPath. #13282 — Enlighten GetFrameworkPath and GetFrameworkSdkPath (locking shared static state; for reference)
• Sibling issue: Enlighten FormatUrl task for multithreaded mode #13567 — Enlighten FormatUrl task (structural template for this issue)
• SKILL: .github/skills/multithreaded-task-migration/SKILL.md