-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ffigen] Add Header Files to CBuilder Sources #2098
base: main
Are you sure you want to change the base?
Conversation
- Added .h files to sources list in example/build/native_add_library/hook/build.dart - Documented sources parameter in CBuilder.library constructor to include .h files - Added note to native_assets_cli/README.md about including .h files in sources
The PR resolves the issue: #1332 |
/// Supported by Clang-like compilers, which can optimize with precompiled | ||
/// headers when `.h` files are included. If a compiler does not support this, | ||
/// the build system may filter `.h` files from the compilation step while | ||
/// still tracking them as dependencies. | ||
super.sources = const [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment should go on the field rather than the constructor argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the whole segment be moved or just this portion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We comment fields not constructor params, so all of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup done removed that part
pkgs/native_assets_cli/README.md
Outdated
@@ -50,6 +50,23 @@ experiment is only available on the master channel. | |||
We do breaking changes regularly! So frequently bump `package:native_assets_cli` | |||
and use dev/master SDK for CI. | |||
|
|||
**Note:** When configuring `CBuilder` in your `hook/build.dart`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not have CBuilder documentation here. The native assets cli can be used with other builders. Add the documentation to the CBuilder instead.
/// sources: [ | ||
/// 'src/native_add_library.c', | ||
/// 'src/native_add_library.h', | ||
/// 'src/dart_api_dl.c', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omit dart_api_dl
from the documentation, it doesn't add extra info but it does complicate things. That's not great for documentation.
/// 'src/dart_api_dl.h', | ||
/// ], | ||
/// ``` | ||
/// Supported by Clang-like compilers, which can optimize with precompiled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CBuilder
should work for all currently supported compilers. If this doesn't work for example for MSVC we need to filter the header files. Please remove this comment and test if it works for Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkgs/native_assets_cli/README.md
Outdated
@@ -43,7 +43,7 @@ assets work on Dart stable, but prefer using the dev releases as we regularly | |||
break things. | |||
|
|||
To use native assets in Flutter, use | |||
`flutter config --enable-experiment=native-assets` and then | |||
`flutter config --enable-experiment=native-assets` and t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spurious edit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. 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.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
@dcharkes just a follow up ,i have fixed the areas of concern |
Please go over all the
Also, in many places in the unit tests only the
|
This pull request fixes an issue where changes to header files (
.h
) in native asset builds did not invalidate the build cache, causing stale builds withCBuilder
. As noted by @dcharkes in #1332, thesources
list previously omitted header files, missing their dependency updates. This PR implements Option 1 from the issue: adding.h
files tosources
to ensure rebuilds occur when headers change. Along with updating theREADME.md
for reflecting the necessary changes made.Changes Made
Updated Example and Test Projects
CBuilder
configurations to include.h
files in thesources
list across multiple projects:pkgs/native_assets_cli/example/build/native_add_library/hook/build.dart
:'src/$packageName.h'
and'src/dart_api_dl.h'
.pkgs/native_assets_cli/example/build/use_dart_api/hook/build.dart
:.h
files (e.g.,use_dart_api.h
,dart_api_dl.h
).Documented
CBuilder.sources
Added documentation to the sources parameter in the
CBuilder.library
constructor inpkgs/native_toolchain_c/lib/src/cbuilder/cbuilder.dart
:.c
and.h
files, aligning with the new behavior and Clang compatibility notes from the issue.Added a note to
pkgs/native_assets_cli/README.md
under "Usage"