Skip to content

Improve firehose publish validation log output - #468

Merged
natebosch merged 2 commits into
mainfrom
publish-noise
Oct 14, 2026
Merged

natebosch merged 2 commits into
mainfrom
publish-noise

Conversation

@natebosch

Copy link
Copy Markdown
Member

When the publishing PR comment can't be written, the release links were
not available anywhere in the workflow log, and the log was dominated by
repeated "Found package at ..." lines.

  • Print the links to create GitHub releases for packages that are ready
    to publish, along with any validation errors, as the final output of
    Firehose.validate, outside of any group so they are easy to find.
  • Search the file system for packages at most once per Repository.
    isSinglePackageRepo previously re-ran package discovery on every
    call, and it was called via calculateRepoTag and
    calculateReleaseUri for each package.
  • Group the log output into collapsible sections for package discovery,
    the validation of each package, and preparing the PR comment.
  • Extract the per-package validation into _verifyPackage.
  • Wait for subprocess output to be written before runCommand returns,
    so it isn't interleaved with later output or written outside its log
    group.

When the publishing PR comment can't be written, the release links were
not available anywhere in the workflow log, and the log was dominated by
repeated "Found package at ..." lines.

- Print the links to create GitHub releases for packages that are ready
  to publish, along with any validation errors, as the final output of
  `Firehose.validate`, outside of any group so they are easy to find.
- Search the file system for packages at most once per `Repository`.
  `isSinglePackageRepo` previously re-ran package discovery on every
  call, and it was called via `calculateRepoTag` and
  `calculateReleaseUri` for each package.
- Group the log output into collapsible sections for package discovery,
  the validation of each package, and preparing the PR comment.
- Extract the per-package validation into `_verifyPackage`.
- Wait for subprocess output to be written before `runCommand` returns,
  so it isn't interleaved with later output or written outside its log
  group.
@natebosch
natebosch requested a review from kevmoo October 14, 2026 00:18
@natebosch
natebosch requested a review from mosuem as a code owner October 14, 2026 00:18
@natebosch

Copy link
Copy Markdown
Member Author

@kevmoo pointed out theres a different place you can find these links, but the noise cleanup is still worthwhile IMO, and it doesn't hurt to also print the link at the end of the log output.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request improves the GitHub Actions logging for the publish.yaml workflow by grouping log outputs into collapsible sections, caching package discovery to search the file system at most once per Repository, and printing release links at the end of the validation log. Additionally, it refactors package verification and ensures command output streams are fully written before returning. Feedback suggests concurrently awaiting process.exitCode, stdoutDone, and stderrDone using Future.wait in runCommand to handle potential stream errors more robustly.

Comment thread pkgs/firehose/lib/src/utils.dart Outdated
Comment on lines +46 to +49
final code = await process.exitCode;
// Wait for all output to be written so that it isn't interleaved with any
// output from the caller after this returns.
await Future.wait([stdoutDone, stderrDone]);

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.

medium

Concurrently await process.exitCode, stdoutDone, and stderrDone using Future.wait. This is more robust because if any of the streams encounter an error (e.g., decoding issues) while the process is still running, the error will be caught and propagated immediately rather than being unhandled or delayed.

Suggested change
final code = await process.exitCode;
// Wait for all output to be written so that it isn't interleaved with any
// output from the caller after this returns.
await Future.wait([stdoutDone, stderrDone]);
final results = await Future.wait([
process.exitCode,
stdoutDone,
stderrDone,
]);
final code = results[0] as int;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or use the record fun!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.


- Print the links to create GitHub releases at the end of the `publish.yaml`
validation log so they are available when the PR comment can't be written.
- Group the `publish.yaml` log output into collapsible sections for package

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Love it!

@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

PR Health

Coverage ✔️
File Coverage
pkgs/firehose/lib/firehose.dart 💚 57 % ⬆️ 8 %
pkgs/firehose/lib/src/repo.dart 💚 98 % ⬆️ 0 %
pkgs/firehose/lib/src/utils.dart 💚 90 % ⬆️ 2 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

License Headers ✔️
// Copyright (c) 2026, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

This check can be disabled by tagging the PR with skip-license-check.

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
firehose Breaking 0.13.0 0.13.2-wip 0.13.2-wip ✔️

This check can be disabled by tagging the PR with skip-breaking-check.

Unused Dependencies ✔️
Package Status
firehose ✔️ All dependencies utilized correctly.

For details on how to fix these, see dependency_validator.

This check can be disabled by tagging the PR with skip-unused-dependencies-check.

API leaks ⚠️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources
firehose Pub firehose/firehose.dart::Firehose::verify::pub

This check can be disabled by tagging the PR with skip-leaking-check.

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check (for example, for non-user-visible changes when the package version is already at -wip).

@kevmoo kevmoo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@natebosch – posted unverified silly then deleted it!

@natebosch

Copy link
Copy Markdown
Member Author

posted unverified silly then deleted it!

?

@kevmoo

kevmoo commented Oct 14, 2026

Copy link
Copy Markdown
Member

posted unverified silly then deleted it!

?

never mind 🙃

@natebosch
natebosch merged commit 44a0768 into main Oct 14, 2026
27 checks passed
@natebosch
natebosch deleted the publish-noise branch October 14, 2026 16:37
copybara-service Bot pushed a commit to dart-lang/sdk that referenced this pull request Oct 20, 2026
…, web

Revisions updated by `dart tools/rev_sdk_deps.dart`.

core (https://github.com/dart-lang/core/compare/4d80610..72e71ca):
  72e71ca0  Mon Oct 19 21:57:11 2026 +0200  Jonas Finnemann Jensen  Optimize `HashSink` and `_MD5Sink.updateHash` (RFC impl) (dart-lang/core#1013)

dartdoc (https://github.com/dart-lang/dartdoc/compare/e1859cf..c0e19cf):
  c0e19cf6  Mon Oct 19 11:41:42 2026 -0700  Konstantin Scheglov  Import analyzer ast with explicit show. (dart-lang/dartdoc#4300)

ecosystem (https://github.com/dart-lang/ecosystem/compare/622ef61..0ab0cfd):
  0ab0cfd  Mon Oct 19 15:44:26 2026 +0200  Moritz  fix(firehose): bump dependency_validator to support analyzer 14 (dart-lang/ecosystem#453)
  44a0768  Wed Oct 14 09:37:29 2026 -0700  Nate Bosch  Improve firehose publish validation log output (dart-lang/ecosystem#468)

http (https://github.com/dart-lang/http/compare/7d7aae2..a8a24a0):
  a8a24a0  Mon Oct 19 13:31:34 2026 -0700  Kevin Moore  docs(web_socket): remove non-user-visible changelog entry (dart-lang/http#2023)
  6f6ac5f  Mon Oct 19 16:29:43 2026 -0400  Mouad Debbar  [http_multi_server] Retry `loopback(0)` when IPv6 port is already bound (dart-lang/http#2025)
  6e91ce3  Tue Oct 13 16:25:39 2026 -0700  Kevin Moore  Sync root README.md package table from pubspec.yaml files (dart-lang/http#2024)

native (https://github.com/dart-lang/native/compare/cc7e4b0..3cbe319):
  3cbe319e3  Tue Oct 13 15:47:18 2026 +0200  Daco Harkes  [hooks_runner] Fail eagerly when file locking is not supported on file system (dart-lang/native#3679)

shelf (https://github.com/dart-lang/shelf/compare/b2cb503..b66665b):
  b66665b  Fri Oct 16 15:12:05 2026 -0700  Kevin Moore  chore: add health workflow and prepare shelf 1.4.3, shelf_router 1.1.5, and shelf_static 1.2.0 (dart-lang/shelf#546)

test (https://github.com/dart-lang/test/compare/9bf69a8..d1afeb1):
  d1afeb14  Fri Oct 16 20:41:51 2026 -0400  Mouad Debbar  Confirmation that `tearDown` is called on timeout (dart-lang/test#2798)
  3f6c2f14  Thu Oct 15 18:21:35 2026 -0700  Jacob MacDonald  Enable asserts in the cli compiler (dart-lang/test#2795)

tools (https://github.com/dart-lang/tools/compare/c1471db..f6c7a2d):
  f6c7a2d62  Mon Oct 19 07:59:24 2026 -0700  Kevin Moore  [pubspec_parse] Preserve input maps and YamlMap source spans (dart-lang/tools#2653)
  f1a55d953  Mon Oct 19 07:53:51 2026 -0700  Kevin Moore  [yaml] Fix tag URI decoding, numeric tag RangeErrors, and dead checks (dart-lang/tools#2630)
  340ded284  Mon Oct 19 07:43:31 2026 -0700  Kevin Moore  [unified_analytics] Prepare for 8.2.0 release (dart-lang/tools#2689)
  a73703543  Fri Oct 16 11:55:03 2026 -0700  Kevin Moore  [unified_analytics] Bump version to 8.2.0-wip (dart-lang/tools#2688)

web (https://github.com/dart-lang/web/compare/63e6662..4e60e61):
  4e60e61  Mon Oct 19 10:02:22 2026 -0700  dependabot[bot]  Bump source-map-js from 1.2.1 to 1.2.2 in /web_generator (dart-lang/web#591)
  7d63637  Mon Oct 19 09:50:26 2026 -0700  Kevin Moore  Add an api.txt golden for package:web (dart-lang/web#590)

[email protected]

Change-Id: Ibe6ac8a8246a27caa8a4846ea1acc78c801ee2ae
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/566781
Commit-Queue: Nate Bosch <[email protected]>
Reviewed-by: Brian Quinlan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants