Repository navigation
[pigeon] Fix basePath resolution when paths share a string prefix with basePath - #13342
tarrinneal wants to merge 5 commits into
Conversation
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
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
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.
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].///).