Skip to content

Conversation

@davidmorgan
Copy link
Contributor

This is currently blocking fixnum rolls to google3; having discussed various fixes/workarounds, a highly targeted hard coded fix seems appealing :) ... what do you think, please?

@github-actions
Copy link

Package publishing

Package Version Status Publish tag (post-merge)
package:bazel_worker 1.1.5 already published at pub.dev
package:benchmark_harness 2.4.0 already published at pub.dev
package:boolean_selector 2.1.2 already published at pub.dev
package:browser_launcher 1.1.3 already published at pub.dev
package:cli_config 0.2.1-wip WIP (no publish necessary)
package:cli_util 0.5.0-wip WIP (no publish necessary)
package:clock 1.1.3-wip WIP (no publish necessary)
package:code_builder 4.11.1 ready to publish code_builder-v4.11.1
package:coverage 1.15.0 already published at pub.dev
package:csslib 1.0.2 already published at pub.dev
package:extension_discovery 2.1.0 already published at pub.dev
package:file 7.0.2-wip WIP (no publish necessary)
package:file_testing 3.1.0-wip WIP (no publish necessary)
package:glob 2.1.3 already published at pub.dev
package:graphs 2.3.3-wip WIP (no publish necessary)
package:html 0.15.7-wip WIP (no publish necessary)
package:io 1.1.0-wip WIP (no publish necessary)
package:json_rpc_2 4.1.0-wip WIP (no publish necessary)
package:markdown 7.3.1-wip WIP (no publish necessary)
package:mime 2.1.0-wip WIP (no publish necessary)
package:oauth2 2.0.5 already published at pub.dev
package:package_config 2.3.0-wip WIP (no publish necessary)
package:pool 1.5.2 already published at pub.dev
package:process 5.0.5 already published at pub.dev
package:pub_semver 2.2.0 already published at pub.dev
package:pubspec_parse 1.5.1-wip WIP (no publish necessary)
package:source_map_stack_trace 2.1.3-wip WIP (no publish necessary)
package:source_maps 0.10.14-wip WIP (no publish necessary)
package:source_span 1.10.1 already published at pub.dev
package:sse 4.1.8 already published at pub.dev
package:stack_trace 1.12.2-wip (error) pubspec version (1.12.2-wip) and changelog (1.12.2-dev) don't agree
package:stream_channel 2.1.4 already published at pub.dev
package:stream_transform 2.1.2-wip WIP (no publish necessary)
package:string_scanner 1.4.1 already published at pub.dev
package:term_glyph 1.2.3-wip WIP (no publish necessary)
package:test_reflective_loader 0.5.0 ready to publish test_reflective_loader-v0.5.0
package:timing 1.0.2 already published at pub.dev
package:unified_analytics 8.0.9 ready to publish unified_analytics-v8.0.9
package:watcher 1.2.0 already published at pub.dev
package:yaml 3.1.3 already published at pub.dev
package:yaml_edit 2.2.3 already published at pub.dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

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 introduces a targeted fix to handle upcoming changes in fixnum v1.2.0 by rewriting imports from package:fixnum/src/* to package:fixnum/fixnum.dart. The changes are implemented in the Allocator by introducing a _fixUrl helper function, and the package version is updated accordingly. The changes are well-tested and documented in the changelog. My feedback includes a suggestion to improve maintainability by extracting magic strings into constants, following Effective Dart guidelines.

@github-actions
Copy link

PR Health

License Headers ✔️
// Copyright (c) 2025, 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.

Unrelated files missing license headers
Files
pkgs/bazel_worker/benchmark/benchmark.dart
pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart
pkgs/boolean_selector/example/example.dart
pkgs/clock/lib/clock.dart
pkgs/clock/lib/src/clock.dart
pkgs/clock/lib/src/default.dart
pkgs/clock/lib/src/stopwatch.dart
pkgs/clock/lib/src/utils.dart
pkgs/clock/test/clock_test.dart
pkgs/clock/test/default_test.dart
pkgs/clock/test/stopwatch_test.dart
pkgs/clock/test/utils.dart
pkgs/coverage/lib/src/coverage_options.dart
pkgs/html/example/main.dart
pkgs/html/lib/dom.dart
pkgs/html/lib/dom_parsing.dart
pkgs/html/lib/html_escape.dart
pkgs/html/lib/parser.dart
pkgs/html/lib/src/constants.dart
pkgs/html/lib/src/encoding_parser.dart
pkgs/html/lib/src/html_input_stream.dart
pkgs/html/lib/src/list_proxy.dart
pkgs/html/lib/src/query_selector.dart
pkgs/html/lib/src/token.dart
pkgs/html/lib/src/tokenizer.dart
pkgs/html/lib/src/treebuilder.dart
pkgs/html/lib/src/utils.dart
pkgs/html/test/dom_test.dart
pkgs/html/test/parser_feature_test.dart
pkgs/html/test/parser_test.dart
pkgs/html/test/query_selector_test.dart
pkgs/html/test/selectors/level1_baseline_test.dart
pkgs/html/test/selectors/level1_lib.dart
pkgs/html/test/selectors/selectors.dart
pkgs/html/test/support.dart
pkgs/html/test/tokenizer_test.dart
pkgs/html/test/trie_test.dart
pkgs/html/tool/generate_trie.dart
pkgs/pubspec_parse/test/git_uri_test.dart
pkgs/stack_trace/example/example.dart
pkgs/watcher/test/custom_watcher_factory_test.dart
pkgs/yaml_edit/example/example.dart

This check can be disabled by tagging the PR with skip-license-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

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

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
code_builder None 4.11.0 4.11.1 4.11.0 ✔️

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

Coverage ✔️
File Coverage
pkgs/code_builder/lib/src/allocator.dart 💚 100 %

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.

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.

@davidmorgan davidmorgan marked this pull request as ready for review December 12, 2025 16:12
@davidmorgan davidmorgan requested a review from a team as a code owner December 12, 2025 16:12
@mosuem
Copy link
Member

mosuem commented Dec 12, 2025

Happy to rubberstamp, but why would this fix be needed? Imports of src/ files can just be broken, as they are not subject to semantic versioning.

@natebosch
Copy link
Member

a highly targeted hard coded fix seems appealing

Do we plan to back out the fix in the future? If it's temporary I think I'd prefer landing it in google3 and noted as a local modification, then landing the long term fix here.

My initial reaction is that we should push the mapping further towards whatever codegen is choosing that URI, but I think I need to read through the internal bug thread to figure out why we aren't doing that.


/// Applies hardcoded fixes to [url].
String _fixUrl(String url) {
if (url.startsWith('package:fixnum/src/')) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a link to the issue here? This feels like the kind of crazy that we forget why we did in 3 years.

@natebosch
Copy link
Member

After reading the thread I'm not convinced that a hardcoded string based fix in package:code_builder is the right long term solution. I don't think we should land this externally.

@davidmorgan
Copy link
Contributor Author

Thanks everyone! This is a fun one. Let's discuss on the internal issue, I'll add everyone.

## 4.11.1-wip
## 4.11.1

* Convert imports of implementation libraries under `package:fixnum/src/*` into
Copy link
Member

Choose a reason for hiding this comment

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

Can we also mention that this is expected to be removed in some future release?

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.

4 participants