Repository navigation
[pigeon] Fix basePath resolution when paths share a string prefix with basePath - #13342
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper function isWithinOrEquals in generator_tools.dart to resolve issues where basePath is incorrectly matched when an output or directory path shares a string prefix with it. This helper replaces startsWith checks across multiple generators, and corresponding unit tests are added. The reviewer suggests explicitly checking that internalOptions.basePath is not empty before calling isWithinOrEquals in pigeon_lib.dart to avoid fragile behavior with empty strings.
| (internalOptions.basePath != null && | ||
| targetConfigDir != null && | ||
| targetConfigDir.isNotEmpty && | ||
| !targetConfigDir.startsWith(internalOptions.basePath!)) | ||
| !isWithinOrEquals(internalOptions.basePath!, targetConfigDir)) |
There was a problem hiding this comment.
To ensure consistency with other generators and avoid relying on fragile behavior of path.isWithin when called with an empty string, we should explicitly check that internalOptions.basePath is not empty before calling isWithinOrEquals.
(internalOptions.basePath != null &&
internalOptions.basePath!.isNotEmpty &&
targetConfigDir != null &&
targetConfigDir.isNotEmpty &&
!isWithinOrEquals(internalOptions.basePath!, targetConfigDir))| (internalOptions.basePath != null && | ||
| targetConfigDir != null && | ||
| targetConfigDir.isNotEmpty && | ||
| !targetConfigDir.startsWith(internalOptions.basePath!)) | ||
| !isWithinOrEquals(internalOptions.basePath!, targetConfigDir)) |
There was a problem hiding this comment.
To ensure consistency with other generators and avoid relying on fragile behavior of path.isWithin when called with an empty string, we should explicitly check that internalOptions.basePath is not empty before calling isWithinOrEquals.
(internalOptions.basePath != null &&
internalOptions.basePath!.isNotEmpty &&
targetConfigDir != null &&
targetConfigDir.isNotEmpty &&
!isWithinOrEquals(internalOptions.basePath!, targetConfigDir))
davidmorgan
left a comment
davidmorgan
left a comment
There was a problem hiding this comment.
Thanks, this fixes the app_bindings case. One question about the approach, and one pre-existing issue nearby.
Output paths (dartOut, swiftOut) are never pre-joined with basePath, and _getFile always writes to path.posix.join(basePath, out). So the config is correct exactly when it also uses the plain join, which already returns out unchanged when out is absolute. With the "already inside" skip, a relative output that really is under basePath still ends up wrong, before and after this PR. For example, with basePath: 'app' and dartOut: 'app/lib/x.dart', the file is written to app/app/lib/x.dart but the config points at app/lib/x.dart. The check is only needed for the top-level appDirectory/configDirectory, which InternalPigeonOptions pre-joins.
Pre-existing, not from this PR: the per-language swiftOptions.configDirectory and kotlinOptions.configDirectory aren't pre-joined. shouldGenerate writes the config file under them without basePath, but pigeon_lib.dart runs ffigen/jnigen in join(basePath, configDirectory). So with basePath set, the config file is written in one place and the tool runs in another.
| (basePath.isNotEmpty && swiftOutPath.isNotEmpty && !swiftOutPath.startsWith(basePath)) | ||
| (basePath.isNotEmpty && | ||
| swiftOutPath.isNotEmpty && | ||
| !isWithinOrEquals(basePath, swiftOutPath)) |
There was a problem hiding this comment.
For output paths, is the skip needed at all? _getFile always joins, so path.posix.join(basePath, swiftOutPath) would match the written file in every case, absolute paths included. As written, swiftOut: 'app/Messages.g.swift' under basePath: 'app' is still mismatched. And ./app/Messages.g.swift was joined correctly before, but is now treated as already inside, so the config path doesn't match the written file. Same for dartOutPath below and in jnigen_config_generator.dart.
davidmorgan
left a comment
davidmorgan
left a comment
There was a problem hiding this comment.
One more site with the same prefix bug, missed in the merge with main: _checkGeneratedBindingsPubspecVersion in pigeon_lib_internal.dart (line 2779 at 3999bc2) still uses !dartOutPath.startsWith(basePath). With basePath: 'app' and dartOut: 'app_bindings/lib/x.dart', it looks for the bindings file at app_bindings/... instead of app/app_bindings/..., finds nothing and skips the version check without saying so. It probably wants isWithinOrEquals like the other sites.
# Conflicts: # packages/pigeon/CHANGELOG.md # packages/pigeon/lib/src/generator_tools.dart # packages/pigeon/pubspec.yaml
tarrinneal
added
the
autosubmit
…er#196381) flutter/packages@08faf64...653628e 2026-11-11 [email protected] Roll Flutter from fffd8c2 to b98afb5 (31 revisions) (flutter/packages#13623) 2026-11-11 [email protected] [material_ui] Document how to programmatically clear DropdownMenu selection (flutter/packages#13266) 2026-11-11 [email protected] [pigeon] Fix basePath resolution when paths share a string prefix with basePath (flutter/packages#13342) 2026-11-11 [email protected] [cupertino_ui] Fix CupertinoSheetRoute throwing when a drag ends during a pending relayout (flutter/packages#13316) 2026-11-11 [email protected] [shared_preferences] Updates Pigeon to 30.0.1 to support multi-engine registration and deregistration. (flutter/packages#13617) 2026-11-11 [email protected] [vector_graphics] Catch any throwable in useHtmlRenderObject() and cache HTML fallback (flutter/packages#13545) 2026-11-11 [email protected] [cupertino_ui] Include selectionHandleColor in NoDefaultCupertinoThemeData equality (flutter/packages#13450) 2026-11-11 [email protected] [flutter_svg] Remove failed async loads from Cache._pending (flutter/packages#13604) 2026-11-11 [email protected] [pigeon] Adds Native Interop FFI codec serialization for NS types (flutter/packages#13616) 2026-11-10 [email protected] [pigeon] Support multi-engine Native Interop registration and instance-aware unregistration (flutter/packages#13576) 2026-11-10 [email protected] [image_picker_ios] Convert the iOS example app and UI tests to Swift (flutter/packages#13570) 2026-11-10 [email protected] [cupertino_ui] Localize CupertinoActionSheet semantics label (flutter/packages#13377) 2026-11-10 [email protected] [camera_android] Fix stopVideoRecording returning JPEG when takePicture runs during recording (flutter/packages#13070) 2026-11-10 [email protected] [google_sign_in_android] Rename keep.xml to google_sign_in_keep.xml (flutter/packages#13475) 2026-11-10 [email protected] [web_benchmarks] Fix benchmark stalling when only repainting views that are dirty (flutter/packages#13089) 2026-11-10 [email protected] Roll Flutter from d463821 to fffd8c2 (21 revisions) (flutter/packages#13614) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Replaces string-prefix
!....startsWith(basePath)checks acrossffigen_config_generator.dart,jnigen_config_generator.dart,pigeon_lib.dart, andpigeon_lib_internal.dartwith a path-awareisWithinOrEquals(basePath, ...)helper usingpath.posix.equalsandpath.posix.isWithin.Previously, when
basePathwas set (e.g.basePath: 'app') and an output or directory path happened to start with the same string prefix without being insidebasePath(e.g.dartOut: 'app_bindings/lib/src/messages.g.dart'),.startsWith(basePath)evaluated totrueand skipped prependingbasePath, while_getFileunconditionally joinedbasePathandoutput.Follow-up from #13283 (review).
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [versioning exemption] this PR falls under[^1].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [changelog exemption] this PR falls under[^1].///).