[bgen] Remove source compilation support - #26614
rolfbjarne wants to merge 4 commits into
Conversation
Require compiled API definition assemblies and leave generated source compilation to consumers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved bgen output validation and CLI contract issues, plus test-harness failures, block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates bgen to consume precompiled API-definition assemblies and generate binding sources only.
Changes:
- Removes internal compilation paths and obsolete options.
- Updates MSBuild, Makefile, tests, diagnostics, and documentation.
- Adds external compilation support to the test harness.
File summaries
| File | Summary |
|---|---|
tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/BGenTaskTest.cs |
Updates task argument tests. |
tests/bgen/ErrorTests.cs |
Tests removed options and source support. |
tests/bgen/BGenTool.cs |
Adds external compilation steps. |
src/Resources.resx |
Removes obsolete diagnostics. |
src/Resources.Designer.cs |
Removes generated obsolete resources. |
src/Makefile |
Updates generator flags. |
src/bgen/Models/BindingTouchConfig.cs |
Removes obsolete compiler configuration. |
src/bgen/DocumentationManager.cs |
Updates compilation documentation. |
src/bgen/BindingTouch.cs |
Removes compilation paths and uses compiled assemblies. |
msbuild/Xamarin.Shared/Xamarin.Shared.targets |
Supplies compiled API assemblies. |
msbuild/Xamarin.MacDev.Tasks/Tasks/BGen.cs |
Updates task arguments and validation. |
docs/website/generator-errors.md |
Removes obsolete error documentation. |
Review details
Files not reviewed (1)
- src/Resources.Designer.cs: Generated file
Suppressed comments (4)
msbuild/Xamarin.MacDev.Tasks/Tasks/BGen.cs:210
- ❌ error — Once bgen only generates source,
GeneratedSourcesFileListis required too. With it empty this command omits/sourceonly; the local task returns success without exposing generated files (and the remote path later tries to read an empty filename). Validate it here and mark the task parameter required.
if (CompiledApiDefinitionAssembly is null || string.IsNullOrEmpty (CompiledApiDefinitionAssembly.ItemSpec)) {
Log.LogError ("A compiled API definition assembly is required.");
return false;
src/bgen/BindingTouch.cs:138
⚠️ warning — The source-only workflow still treats--sourceonlyas optional. If a caller follows the help and supplies only the required compiled assembly,TryGeneratewrites files under the temporary directory and deletes it infinally, so bgen exits successfully with no usable output. Require a persistent--outdir/--sourceonlycombination or update the CLI contract and help.
{ "sourceonly=", "Writes the generated source file list", v => config.GeneratedFileList = v },
tests/bgen/BGenTool.cs:197
- ❌ bug — The harness now compiles the API source before running bgen.
BI0086intentionally usesProfile.Nonewith no target framework (ErrorTests.cs:44-47), so thisCompilecall has no target-framework-derived BCL/base/attribute references but still adds thenfloatglobal using; the compiler can fail before bgen emits BI0086 and the existing regression test reports the wrong failure. Validate the bgen command before compiling or provide a valid reference set for this case.
var compileApiDefinitionsResult = Compile (CompiledApiDefinitionAssembly, ApiDefinitions.Concat (Sources), true);
tests/bgen/BGenTool.cs:233
- ❌ Bug — This new post-generation compile path unconditionally calls
AssemblyPath, but that property still derives its default fromApiDefinitions[0]. A caller using the intended precompiled-API workflow can provideCompiledApiDefinitionAssemblywith no API source files, causing anArgumentOutOfRangeExceptionafter bgen succeeds; derive the final output path fromOutor a neutral default and add a source-less external-assembly test.
var compileResult = Compile (AssemblyPath, File.ReadAllLines (GetGeneratedSourcesFileList ()).Concat (Sources).Concat (ExtraSources), false);
- Files reviewed: 11/12 changed files
- Comments generated: 1
- Review effort level: Lite
Require persistent generated source outputs and keep test harness diagnostics and precompiled assembly flows working. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This will be .NET 12, so waiting until .NET 11 RC 2 has been branched. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical warning-suppression and output-path validation issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (1)
- src/Resources.Designer.cs: Generated file
Suppressed comments (3)
msbuild/Xamarin.MacDev.Tasks/Tasks/BGen.cs:177
⚠️ test_coverage — The new hard requirement forCompiledApiDefinitionAssemblyhas no directExecute()regression test:GeneratedSourceOutputsAreRequiredsets this property and only covers the second validation branch. Add a test that supplies valid generated-source outputs but leaves the compiled assembly unset, and assert the task fails/logs the required-assembly error, so source-mode execution cannot silently return.
if (CompiledApiDefinitionAssembly is null || string.IsNullOrEmpty (CompiledApiDefinitionAssembly.ItemSpec)) {
Log.LogError ("A compiled API definition assembly is required.");
return false;
src/bgen/BindingTouch.cs:314
- ❌ bug — This guard distinguishes only
nullfrom a usable output directory.--outdir=is parsed as an empty string, so it passes here;TryGeneratethen uses that empty value asGenerator.BaseDirand writes generated files relative to the process working directory instead of rejecting the invalid output path. Treat empty values as absent (or reject them while parsing).
if (config.BindingFilesOutputDirectory is null && config.DeleteTemporaryFiles) {
tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/BGenTaskTest.cs:18
- 💡 test_coverage —
Execute()now requiresGeneratedSourcesDiralongside the list, but this test leaves the directory unset and therefore never verifies the new/tmpdir:switch. Populate the directory in this setup and assert it is emitted so the task's source-output contract is covered.
task.GeneratedSourcesFileList = Path.Combine (Cache.CreateTemporaryDirectory (), "generated-sources.txt");
- Files reviewed: 12/13 changed files
- Comments generated: 1
- Review effort level: Lite
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Keep compiled API definitions in a separate directory so that generated binding assemblies do not overwrite the input assembly. Preserve the API assembly basename used for generated helper namespaces, initialize the response-file test profile, and update XML documentation ordering for the sorted generated source list. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd2e6ad9-8e15-400a-ad9b-4c2150fee229
✅ 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 |
🔥 [CI Build #5b88546] Test results 🔥Test results❌ Tests failed on VSTS: test results 0 tests crashed, 2 tests failed, 262 tests passed. Failures❌ generator tests1 tests failed, 4 tests passed.Failed tests
Html Report (VSDrops) Download ❌ windows tests1 tests failed, 2 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 |
🤖 Pull request created by Copilot