Repository navigation
[ffigen] Add Input.appendCompilerOptions to append clang args to defaults (#3630) - #3676
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@googlebot I signed it! |
|
Hi @liamappelbe and @dcharkes, when you get a chance, could you please take a look at my PR #3676 and let me know if there’s anything I should change or improve? I’d really appreciate your feedback. Thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3676 +/- ##
=======================================
Coverage 88.13% 88.13%
=======================================
Files 306 306
Lines 19507 19511 +4
=======================================
+ Hits 17192 17196 +4
Misses 2315 2315
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
liamappelbe
left a comment
liamappelbe
left a comment
There was a problem hiding this comment.
Looks good. Just some minor comments.
| @@ -1,5 +1,7 @@ | |||
| ## 22.0.1-wip | |||
|
|
|||
| - Added `Input.appendCompilerOptions` to allow appending custom compiler | |||
There was a problem hiding this comment.
This is adding a new API, so we should upgrade the new version to a minor version bump (here and in the pubspec.yaml): 22.1.0-wip
There was a problem hiding this comment.
Done! i had upgragded the version to 22.1.0-wip in both pubspec.yaml and CHANGELOG.md
| late final compilerOpts = computeCompilerOpts( | ||
| input: config.input, | ||
| logger: logger, | ||
| cpp: config.cpp != null, |
There was a problem hiding this comment.
Just pass in the entire config, rather than the input and cpp separately.
There was a problem hiding this comment.
hmm got it.
i have updated computeCompilerOpts to take the entire config object instead of separate parameters, and updated the tests accordingly.
| this.entryPoints = const [], | ||
| this.include = _includeDefault, | ||
| this.compilerOptions, | ||
| this.appendCompilerOptions = false, |
There was a problem hiding this comment.
This is defaulting to false, but your PR description says it defauts to true. I think false is the right approach (otherwise it's a breaking change for existing users). So update the PR description.
There was a problem hiding this comment.
Good catch!
I’ve updated the PR description to clarify that it defaults to false to avoid breaking changes.
| @@ -180,13 +180,18 @@ final class Input { | |||
| /// Command line arguments to pass to clang_compiler. | |||
There was a problem hiding this comment.
Can you document the behaviour as it relates to appendCompilerOptions? If people are reading this on the API docs, they won't see the comment below.
There was a problem hiding this comment.
Done!
Added documentation on compilerOptions explaining that it replaces default compiler options unless appendCompilerOptions is set to true.
- Bump version to 22.1.0-wip in pubspec.yaml and CHANGELOG.md. - Pass entire config to computeCompilerOpts. - Document interaction between compilerOptions and appendCompilerOptions.
|
Hi @liamappelbe, I’ve addressed all the review comments and pushed the changes. |
Revisions updated by `dart tools/rev_sdk_deps.dart`. native (https://github.com/dart-lang/native/compare/f628b26..27f7678): 27f767827 Mon Sep 28 14:51:28 2026 +0200 Daco Harkes [hooks_runner] Remove `KernelAssets` and `Target` (dart-lang/native#3703) ddc7d6deb Mon Sep 28 05:40:03 2026 +0200 Jakob Kordež [ffigen] Skip typedefs to function pointers with unsupported parameter types (dart-lang/native#3694) 177d283d9 Mon Sep 28 04:30:28 2026 +0200 Jakob Kordež [ffigen] Generate bindings for C++ enums, structs and unions nested in namespaces and records (dart-lang/native#3644) 5ee8f67d8 Mon Sep 28 11:39:14 2026 +1000 Liam Appelbe [objective_c] Clarify block isolate scoping (dart-lang/native#3692) 25f090116 Fri Sep 25 12:09:12 2026 +1000 Liam Appelbe [ffigen][jnigen] Deprecate YAML config (dart-lang/native#3654) 8364413eb Fri Sep 25 10:09:50 2026 +1000 Liam Appelbe [jnigen] Document nullability (dart-lang/native#3690) e8d518574 Thu Sep 24 18:02:46 2026 +1000 Liam Appelbe [ffigen] Code snippet-ify ffigen and objective_c (dart-lang/native#3685) 663385955 Thu Sep 24 03:21:53 2026 +0300 Manar Elhabbal [ffigen] prevent setting both cpp and objectiveC in ffigenerator (dart-lang/native#3687) 1cf40952a Thu Sep 24 09:27:09 2026 +1000 Liam Appelbe [objective_c] Remove arm64e build hack (dart-lang/native#3680) c44550b6e Wed Sep 23 13:01:33 2026 +0200 Simon Binder [web_assets] Scaffold package (dart-lang/native#3610) 57b4bad69 Wed Sep 23 16:21:27 2026 +1000 Liam Appelbe [swift2objc] Code snippet-ify swift2objc (dart-lang/native#3684) 5cee1d0be Wed Sep 23 10:16:29 2026 +0530 Rachit Vikram Singh [ffigen] Add Input.appendCompilerOptions to append clang args to defaults (`#3630`) (dart-lang/native#3676) 225a648df Wed Sep 23 14:37:13 2026 +1000 Liam Appelbe [jnigen] Code snippet-ify jnigen and co (dart-lang/native#3641) 2030decf9 Wed Sep 23 13:51:24 2026 +1000 Liam Appelbe [infra] Move snippets_tool to its own package (dart-lang/native#3683) 6fa2beea2 Wed Sep 23 12:14:28 2026 +1000 Liam Appelbe [objective_c] Revert `#3664` (dart-lang/native#3681) ed7605f8a Tue Sep 22 12:03:22 2026 +0530 Sivapriya [jni] Add deep conversion utils (dart-lang/native#3635) 8574b7422 Tue Sep 22 08:31:11 2026 +0200 Lukas Klingsbo [ffigen] Allow package:cli_util 0.5.x and 0.6.x (dart-lang/native#3673) test (https://github.com/dart-lang/test/compare/9946c95..4f92d53): 4f92d530 Fri Sep 25 16:46:22 2026 -0700 Nate Bosch Unskip a test on windows (dart-lang/test#2772) 7fb84866 Fri Sep 25 14:52:39 2026 -0700 Nate Bosch Bundle skips at the end of the GitHub reporter (dart-lang/test#2768) 603e5ec5 Thu Sep 24 14:00:41 2026 -0700 Nate Bosch Clean up the runner's temporary directories (dart-lang/test#2763) 4bf04d90 Thu Sep 24 12:41:15 2026 -0700 Nate Bosch Refactor signal handling in executable.dart (dart-lang/test#2765) 2c660a84 Tue Sep 22 15:33:34 2026 -0700 Nate Bosch Regenerate mono_repo output (dart-lang/test#2767) tools (https://github.com/dart-lang/tools/compare/3614ce0..d87eaf7): d87eaf79 Thu Sep 24 08:31:33 2026 -0700 Kevin Moore fix(benchmark_harness): migrate subprocess spawning to package:cli_util (dart-lang/tools#2585) 8ccf8ccf Wed Sep 23 05:09:10 2026 +0530 Aman Maurya [string_scanner] Extract a mixin for LineScanner and deduplicate eager scanning logic (dart-lang/tools#2569) afcdd6d9 Tue Sep 22 15:38:07 2026 +0200 Morgan :) Document that watch events converge on filesystem state. (dart-lang/tools#2607) f2157cbc Tue Sep 22 04:52:58 2026 +0300 Hasan Armoush [code_builder] Add null-aware cascades, cascade index, and fluent cascade helpers (dart-lang/tools#2540) web (https://github.com/dart-lang/web/compare/e5c6c02..ec71a4c): ec71a4c Mon Sep 28 10:07:01 2026 -0700 Kevin Moore [js_interop_gen] Require `code_builder: ^4.12.0` and update golden headers (dart-lang/web#576) [email protected] Change-Id: I766bc5b2b1297e3251990821bb73387ba685e39b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/556320 Commit-Queue: Nate Bosch <[email protected]> Reviewed-by: Brian Quinlan <[email protected]>
Description
Currently, setting custom
compiler-optsinffigencompletely overwrites the default clang arguments (which include system header paths, host OS/architecture flags, and standard language flags like C++98 / C99). Most users only want to append extra arguments rather than replicate all of ffigen's default clang argument discovery logicThis PR addresses this by implementing Option 2 from #3630:
Input.appendCompilerOptions(bool, defaults tofalse) to allow appending user-specified compiler options to the default clang flags.appendCompilerOptionsisfalse, user-provided options replace the defaults, preserving backward compatibility.computeCompilerOptsinconfig_types.dartto calculate the default compiler arguments based on language, target OS, and system headers.CHANGELOG.mdand integration tests.Related Issues
Fixes #3630
PR Checklist
dart tool/ci.dart --alllocally and resolved all issues identified. This ensures the PR is formatted, has no lint errors, and ran all code generators. This applies to the packages part of the toplevelpubspec.yamlworkspace.CHANGELOG.mdfor the relevant packages. (Not needed for small changes such as doc typos).