Skip to content

[bgen] Route output through IToolLog - #26612

Closed
rolfbjarne wants to merge 1 commit into
dev/rolf/remove-bgen-compilationfrom
dev/rolf/route-bgen-through-itoollog
Closed

rolfbjarne wants to merge 1 commit into
dev/rolf/remove-bgen-compilationfrom
dev/rolf/route-bgen-through-itoollog

Conversation

@rolfbjarne

@rolfbjarne rolfbjarne commented Sep 15, 2026

Copy link
Copy Markdown
Member

Refactor bgen diagnostics and normal output to use an injected IToolLog while preserving standalone command-line behavior through ConsoleLog.

This adds BindingTouch.Run (string [] args, IToolLog log, CancellationToken cancellationToken) for in-process callers. It is preparatory work for #26590, which will pass the BGen MSBuild task as the logger when invoking bgen in-process.

This PR is stacked on #26614, which removes bgen's source compilation and subprocess execution support. The change preserves bgen warning/error formatting, verbosity handling, exit codes, unexpected-exception reporting, cancellation behavior, and help output. Focused tests cover ConsoleLog, injected normal/warning/error output, BI0000 errors, and cancellation.

🤖 Pull request created by Copilot

Add an injectable bgen entry point while preserving standalone console behavior, diagnostic formatting, cancellation, and verbosity handling.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 15, 2026 17:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Four unresolved findings remain, including one critical process-termination issue and three moderate correctness or concurrency issues.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request refactors bgen diagnostics and compiler execution around injected IToolLog, adding in-process BindingTouch.Run while preserving standalone CLI behavior.

Changes:

  • Adds logger-aware execution with cancellation and custom HOME support.
  • Routes diagnostics and compiler output through injected logging.
  • Adds focused logging and cancellation tests.

Unresolved findings:

  • Critical (3 votes)src/bgen/Generator.cs:1252: invalid selectors can terminate the host process.
  • Moderate (1 vote)src/bgen/BindingTouch.cs:401: cancellation is not checked during generation.
  • Moderate (1 vote)src/bgen/BindingTouch.cs:126: global warning state is unsafe for concurrent runs.
  • Moderate (1 vote)src/bgen/error.cs:176: warning details may be promoted to MSBuild errors.
File summaries
File Summary
tools/common/IToolLog.cs Preserves standalone console routing.
tools/common/Driver.execution.cs Adds cancellation-aware process execution.
tests/bgen/LoggingTests.cs Tests logging, errors, and cancellation.
src/bgen/Nomenclator.cs Routes nomenclator warnings through the logger.
src/bgen/Generator.cs Uses injected logging for generator diagnostics.
src/bgen/error.cs Routes diagnostics through IToolLog.
src/bgen/BindingTouch.cs Adds in-process execution and cancellation handling.
src/bgen/AttributeManager.cs Stores the diagnostic logger.
Review details

Suppressed comments (3)

src/bgen/BindingTouch.cs:402

  • ⚠️ bug — The token is checked immediately before and after g.Go(), but not while generation runs. A cancellation requested during this synchronous traversal is therefore ignored until the entire generator finishes (and may then proceed to compilation), so the new in-process entry point cannot cancel promptly. Add cancellation checks within the generator's long-running traversal and cover cancellation during generation.
			g.Go ();
			ThrowIfCancellationRequested ();

src/bgen/BindingTouch.cs:126

  • concurrency — Exposing Run for in-process callers makes concurrent invocations possible, but warning severity is still stored in the process-global ErrorHelper.warning_levels dictionary: Main3 clears it, option parsing mutates it, and BindingException constructors read it. Parallel MSBuild tasks can therefore overwrite or clear each other's --warnaserror/--nowarn settings (and race on the dictionary), producing incorrect diagnostics. Keep warning state in the per-run context/logger or serialize Run invocations.
	public static int Run (string [] args, IToolLog log, CancellationToken cancellationToken, string? customHome)

src/bgen/error.cs:177

  • ⚠️ bug — For a warning (error == false), this still sends the stack trace through LogError. The injected MSBuild logger maps IToolLog.LogError(string) to Log.LogError, so a verbose bgen warning is promoted to an MSBuild error and can fail the build. Route warning details through log.Log while retaining LogError for actual errors.
			if (log.Verbosity > 2 && e.StackTrace is not null)
				log.LogError (e.StackTrace);
  • Files reviewed: 8/8 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/bgen/Generator.cs

if (export.Selector.IndexOfAny (invalid_selector_chars) != -1) {
Console.Error.WriteLine ("Export attribute contains invalid selector name: {0}", export.Selector);
BindingTouch.LogError ($"Export attribute contains invalid selector name: {export.Selector}");
@vs-mobiletools-engineering-service2

Copy link
Copy Markdown
Collaborator

✅ API diff for current PR / commit

NET (empty diffs)

✅ API diff vs stable

NET (empty diffs)

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: bdf0c1662bf58cb123f0e5ac84475b5e8ab685e7 [PR build]

@rolfbjarne
rolfbjarne changed the base branch from main to dev/rolf/remove-bgen-compilation September 15, 2026 18:00
@rolfbjarne

Copy link
Copy Markdown
Member Author

Superseded by #26615, which contains the rebased changes on a new branch because updating this PR's published branch would require a prohibited force-push.

@rolfbjarne rolfbjarne closed this Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants