[msbuild] Add extra arguments for ibtool and actool. Fixes #25677. - #26641
rolfbjarne wants to merge 4 commits into
Conversation
Add ACToolExtraArgs and IBToolExtraArgs properties, parse and forward their values to the corresponding Xcode tools, and invalidate incremental resource caches when they change. Document the new properties and cover argument parsing. Fixes #25677 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Follow the MSBuild task nullability convention for optional inputs and snapshot the property before parsing so netstandard flow analysis can prove it is non-null. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
IBToolExtraArgs changes can leave stale outputs; incremental-build coverage and a clearer parsing diagnostic are also needed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds ACToolExtraArgs and IBToolExtraArgs MSBuild properties for passing additional arguments to Xcode tools.
Changes:
- Parses and validates quoted arguments.
- Tracks extra arguments for incremental builds.
- Documents both properties and adds parser tests.
File summaries
| File | Review summary |
|---|---|
tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/IBToolTaskTests.cs |
Adds quoted and invalid argument tests. Incremental-build regression coverage is still needed. |
msbuild/Xamarin.Shared/Xamarin.Shared.targets |
Moderate bug: changing IBToolExtraArgs can leave stale compiled outputs. Additional incremental-build coverage is needed for both tools. |
msbuild/Xamarin.MacDev.Tasks/Tasks/XcodeCompilerToolTask.cs |
Nit: unmatched-quote errors reference internal AdditionalArguments instead of the public MSBuild property. |
docs/building-apps/build-properties.md |
Documents both new properties. |
Review details
Suppressed comments (3)
msbuild/Xamarin.MacDev.Tasks/Tasks/XcodeCompilerToolTask.cs:152
⚠️ error_message — WhenIBToolExtraArgsorACToolExtraArgscontains an unmatched quote, this reportsUnable to parse the 'AdditionalArguments' value.AdditionalArgumentsis only the task's internal property, not the MSBuild setting the user must fix, so the diagnostic is misleading. Pass the public property name into the helper (or use a generic localized message) so the error identifies the setting that needs correction.
Log.LogError (MSBStrings.E7132 /* Unable to parse the 'AdditionalArguments' value: {0} */, additionalArgumentsValue);
msbuild/Xamarin.Shared/Xamarin.Shared.targets:925
⚠️ test_coverage — This new stamp file is the mechanism that promises changingIBToolExtraArgsinvalidates the cached compiler output, but the added tests only exercise argument parsing and never build twice with a changed property. Add an incremental-build regression that verifies_CoreCompileInterfaceDefinitionsreruns afterIBToolExtraArgschanges (and covers the analogousACToolExtraArgspath) so this cache invalidation cannot silently regress.
Inputs="@(InterfaceDefinition);$(_IBToolExtraArgsInput)"
Outputs="$(_IBToolCache)"
DependsOnTargets="_WriteIBToolExtraArgsInput">
msbuild/Xamarin.Shared/Xamarin.Shared.targets:1054
⚠️ test_coverage — TheACToolExtraArgsstamp and deletion path is also new, but no test changes exercise an incremental build where this property changes and confirm_CoreCompileImageAssetsreruns. Add coverage for this target in the same regression suite so staleactooloutputs are detected.
Inputs="@(ImageAsset);$(_TemporaryAppManifest);$(_ACToolExtraArgsInput)"
Outputs="$(_ACTool_PartialAppManifestCache);$(_ACTool_BundleResourceCache)"
DependsOnTargets="_ReadAppManifest;_WriteACToolExtraArgsInput"
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
This comment has been minimized.
This comment has been minimized.
Include the extra-arguments stamp in IBTool's per-resource freshness check so changing IBToolExtraArgs recompiles interface definitions instead of retaining outputs produced with the previous arguments. Add focused timestamp coverage for the regression. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The critical Windows-to-Mac stamp issue must be fixed, and focused ACTool coverage should be added.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
msbuild/Xamarin.MacDev.Tasks/Tasks/XcodeCompilerToolTask.cs:152
⚠️ error_message — When a project setsIBToolExtraArgsorACToolExtraArgsto an invalid quoted string, this reportsAdditionalArgumentsinstead of the property the user configured. That name is only the internal task parameter, so the diagnostic is misleading and cannot be searched back to either documented MSBuild property; use a tool-specific property name or a generic localized message for the public setting.
Log.LogError (MSBStrings.E7132 /* Unable to parse the 'AdditionalArguments' value: {0} */, additionalArgumentsValue);
msbuild/Xamarin.Shared/Xamarin.Shared.targets:1099
- 💡 test_coverage — The new ACTool wiring is not exercised by the added tests: they invoke
AppendAdditionalArgumentsthroughTestIBTool, so a regression that stops forwardingACToolExtraArgsat this call site would still pass. Add a focused ACTool task test that verifies the generated command arguments (including a quoted value and an invalid value).
AdditionalArguments="$(ACToolExtraArgs)"
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
Run the IBTool and ACTool extra-argument stamp writers through the remote build session and copy the files back to Windows so both remote task execution and local incremental checks observe the same inputs. Improve invalid-argument diagnostics to name the public MSBuild property and add focused ACTool coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Two review findings remain regarding diagnostics and ACTool incremental-build coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
msbuild/Xamarin.MacDev.Tasks/Tasks/XcodeCompilerToolTask.cs:156
⚠️ error_message — This diagnostic only echoes the raw property value and drops theFormatExceptionreturned byStringUtils, so an unmatched quote is reported without the actionable “No matching quote found” reason. Keep the property name, but includeex.Messagein the MSBuild error.
if (!StringUtils.TryParseArguments (additionalArgumentsValue, out var additionalArguments, out var _)) {
var propertyName = AdditionalArgumentsPropertyName ?? nameof (AdditionalArguments);
Log.LogError ("Unable to parse the '{0}' value: {1}", propertyName, additionalArgumentsValue);
return false;
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
| Inputs="@(ImageAsset);$(_TemporaryAppManifest);$(_ACToolExtraArgsInput)" | ||
| Outputs="$(_ACTool_PartialAppManifestCache);$(_ACTool_BundleResourceCache)" | ||
| DependsOnTargets="_ReadAppManifest" | ||
| DependsOnTargets="_ReadAppManifest;_WriteACToolExtraArgsInput" |
This comment has been minimized.
This comment has been minimized.
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🔥 [CI Build #0fb2e83] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 1 tests failed, 263 tests passed. Failures❌ xtro tests1 tests failed, 0 tests passed.Failed tests
Html Report (VSDrops) Download Successes✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Sonoma (14): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Add
ACToolExtraArgsandIBToolExtraArgsMSBuild properties so applications can pass additional command-line arguments toactoolandibtool. This enables scenarios such as opting out of Xcode 27's toolchain-based Interface Builder compilation mode.Parse quoted arguments through the existing
StringUtilshelper and report invalid values as MSBuild errors. Track each property's value as an incremental-build input so changing additional arguments recompiles the affected resources instead of reusing stale outputs.Document both properties and add focused coverage for quoted and invalid argument values.
Fixes #25677.
🤖 Pull request created by Copilot