[ios][tools]do not log "bonjour not found" at all (unless verbose) by hellohuanlin · Pull Request #173569 · flutter/flutter

hellohuanlin · 2025-08-11T19:14:23Z

The previous PR #172913 avoids compilation error by not having the "error: " prefix. However, it's still very confusing to have "key not found" printed out in the terminal (either stderr or stdout), despite that it doesn't cause compilation error anymore.

This PR introduces a new flag "skipErrorLog" which skips logging the stderr if set to true. We set it for plist extraction, because "not having bonjour key" should be one of the expected "normal" states

We don't want to pipe it to stdout either, because it would be confusing too. I figured people would still file issue if they see it in terminal, even if it's a completely normal state. But not a strong opinion, so let me know if you disagree.

However, under verbose mode, we would pipe it to stdout. This will be consistent with our previous behavior before macOS 26.

List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.

Fixes #172627

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

gemini-code-assist

Code Review

This pull request introduces a skipErrorLog flag to suppress expected error messages from plutil, which is a good improvement to reduce confusion from logs. The implementation is clean and the accompanying unit tests are thorough. I have one minor suggestion in the integration test file to improve maintainability by using constants for error messages, consistent with the changes in the unit test file.

hellohuanlin · 2025-08-11T19:15:48Z

-    ], allowFail: true);
+    ProcessResult result = runSync(
+      'plutil',
+      <String>['-extract', 'NSBonjourServices', 'xml1', '-o', '-', builtProductsPlist],


this is by dart format after i added the new parameter

hellohuanlin · 2025-08-11T19:16:02Z

-    ], allowFail: true);
+    result = runSync(
+      'plutil',
+      <String>['-extract', 'NSLocalNetworkUsageDescription', 'xml1', '-o', '-', builtProductsPlist],


this is by dart format after i added the new parameter

hellohuanlin · 2025-08-11T19:46:44Z

i should probably CP this too

vashworth · 2025-08-11T21:56:41Z

+    // via stderr (rather than stdout on older macOS), and logging the message
+    // would be confusing (for either stdout or stderr), since not having the
+    // key is one of the expected states.
+    if (!skipErrorLog && resultStderr.isNotEmpty) {


I think we still want these logs to show if in verbose mode. The way this is now, they won't show in verbose.

I think what you'll want to do is that is skipErrorLog is true, don't write it to streamOutput or echoError. Unless we're in verbose mode, then do write it to echoError.

@vashworth my understanding:

First of all, we don't want to show the message in non-verbose mode for sure, since not having the key is an expected state (also the most common state). We don't want to scream this out in almost every flutter run. I think we already agree on that.

Secondly, no matter verbose or not, we shouldn't echoError, since semantically speaking, to our bonjour logic, not having the key is not an error, but one of the expected states - definitely not an error.

Now the question is, under verbose mode, do we want to pipe the stderr message to stdout? I am leaning towards not to do so, because it's still very confusing to see "Could not extract value, error: No value at that key path or invalid key path: NSBonjourServices", which clearly reads like stderr, not stdout (I suspect apple probably also thinks it's weird to be in stdout, so they moved it to stderr on macOS 26). Though this is not a strong opinion (51%-ish), so if you insist, I am open to pipe it to stdout under verbose mode. The new code will look something like:

// ... if (resultStderr.isNotEmpty) { // ... if (skipErrorLog) { // Even if we skip error, we still want to pipe it to stdout under verbose mode. if (verbose) { echo(errorOutput.toString()); } } else { echoError(errorOutput.toString()); }

^ actually, thinking about the 3rd point more, piping it to stdout under verbose mode is probably fine, since we were printing that string already before macOS 26 (i.e. piping to stdout would make it consistent with our previous behavior before macOS 26)

vashworth · 2025-08-12T16:50:19Z

+  /// @param skipErrorLog If set, will skip stderr in non-verbose mode, and pipe
+  ///     stderr to stdout in verbose mode.
+  /// @param workingDirectory The current working directory to run the command.
+  /// @throws [Exception] if the exit code is not 0.


@param and @throws isn't really supported in Dart.

If you want to give definitions to arguments, you can use brackets to refer to arguments: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#leave-breadcrumbs-in-the-comments

For example

/// Run given command ([bin]) in a synchronous subprocess. /// /// If [allowFail] is true, an exception will not be thrown even if the process returns a /// non-zero exit code. Also, `error:` will not be prefixed to the output to prevent Xcode /// complication failures. /// /// If [skipErrorLog] is true, `stderr` from the process will not be output unless in [verbose] /// mode. If in [verbose], pipes `stderr` to `stdout`. /// /// Will throw [Exception] if the exit code is not 0.

oof, gemini isn't a good excuse - i should've double checked.

vashworth

@hellohuanlin LGTM after you update the comment

jmagman · 2025-08-12T17:59:48Z

+        // An example is on macOS 26, plutil reports NSBonjourServices key not found
+        // via stderr (rather than stdout on older macOS), and logging the message
+        // in stderr would be confusing, since not having the key is one of the expected states.
+        if (verbose) {


I'd prefer to always log errorOutput in verbose mode, even in the Bonjour case. There may be other errors that command generates that we will want to see to triage.

@jmagman yes in verbose mode, all will be logged. The logic is:

skipErrorLog { // true for bonjour if verbose { log to stdout ("key not found" shouldn't be an "error" or "warning" - rather one of the expected states) } else { do not log } } else { // don't change for other features log to stderr }

Ah gotcha, I can't read.

auto-submit · 2025-08-15T20:49:46Z

Time to revert pull request flutter/flutter/173569 has elapsed.
You need to open the revert manually and process as a regular pull request.

…bose)" (#173879) Reverts #173569 Sorry I have to revert this one as it didn't work. # Why didn't work From the comment, it looks like `echoError` is only used for verbose mode, and `streamOutput` is used for non-verbose mode (I am still looking into how this is setup) https://github.com/flutter/flutter/blob/e4f27cd09734db6c6ed94e104ab5333c48dfc544/packages/flutter_tools/bin/xcode_backend.dart#L151 # How to fix ## Possible Fix 1 I tested this works, by changing this line: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/bin/xcode_backend.dart#L159 into this: ``` if (!verbose && exitCode == 0 && !skipErrorLog) ``` ## Possible fix 2: Surprisingly this also works: ``` if (!verbose && exitCode == 0) { streamOutput(errorOutput.string().replaceAll('error', ''); } ``` Although we made sure `errorOutput` doesn't start with `error:`, there's an `error:` in the middle of the string. Possibly because of this code in `mac.dart`: https://github.com/flutter/flutter/blob/e4f27cd09734db6c6ed94e104ab5333c48dfc544/packages/flutter_tools/lib/src/ios/mac.dart#L431-L433 # Why we missed it? I tested my initial code code and assumed updated code would work, because I incorrectly thought `echoError` was the one triggered the output in non-verbose mode. I should have verified it.

…lutter#173569) The previous PR flutter#172913 avoids compilation error by not having the "error: " prefix. However, it's still very confusing to have "key not found" printed out in the terminal (either stderr or stdout), despite that it doesn't cause compilation error anymore. This PR introduces a new flag "skipErrorLog" which skips logging the `stderr` if set to true. We set it for plist extraction, because "not having bonjour key" should be one of the expected "normal" states <s> We don't want to pipe it to `stdout` either, because it would be confusing too. I figured people would still file issue if they see it in terminal, even if it's a completely normal state. But not a strong opinion, so let me know if you disagree. </s> However, under verbose mode, we would pipe it to `stdout`. This will be consistent with our previous behavior before macOS 26. *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* Fixes flutter#172627 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

…bose)" (flutter#173879) Reverts flutter#173569 Sorry I have to revert this one as it didn't work. # Why didn't work From the comment, it looks like `echoError` is only used for verbose mode, and `streamOutput` is used for non-verbose mode (I am still looking into how this is setup) https://github.com/flutter/flutter/blob/e4f27cd09734db6c6ed94e104ab5333c48dfc544/packages/flutter_tools/bin/xcode_backend.dart#L151 # How to fix ## Possible Fix 1 I tested this works, by changing this line: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/bin/xcode_backend.dart#L159 into this: ``` if (!verbose && exitCode == 0 && !skipErrorLog) ``` ## Possible fix 2: Surprisingly this also works: ``` if (!verbose && exitCode == 0) { streamOutput(errorOutput.string().replaceAll('error', ''); } ``` Although we made sure `errorOutput` doesn't start with `error:`, there's an `error:` in the middle of the string. Possibly because of this code in `mac.dart`: https://github.com/flutter/flutter/blob/e4f27cd09734db6c6ed94e104ab5333c48dfc544/packages/flutter_tools/lib/src/ios/mac.dart#L431-L433 # Why we missed it? I tested my initial code code and assumed updated code would work, because I incorrectly thought `echoError` was the one triggered the output in non-verbose mode. I should have verified it.

…de (#174001)  This tries #173569 again That PR was reverted, because I didn't know that `streamOutput` was the one used during non-verbose mode The logging logic is a bit convoluted, see: #173887 So this PR simply changes the condition from ``` if (!verbose && exitCode == 0) ``` To ``` if (!verbose && exitCode == 0 && !skipErrorLog) ``` So that if we skipErrorLog (will be true for bonjour features), it doesn't call `streamOutput`. *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* Fixes #172627 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

…de (flutter#174001)  This tries flutter#173569 again That PR was reverted, because I didn't know that `streamOutput` was the one used during non-verbose mode The logging logic is a bit convoluted, see: flutter#173887 So this PR simply changes the condition from ``` if (!verbose && exitCode == 0) ``` To ``` if (!verbose && exitCode == 0 && !skipErrorLog) ``` So that if we skipErrorLog (will be true for bonjour features), it doesn't call `streamOutput`. *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* Fixes flutter#172627 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md