Skip to content

[native_toolchain_c] Changes to header files should be added to the dependencies #1332

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

Open
3 tasks
dcharkes opened this issue Jul 12, 2024 · 3 comments
Open
3 tasks
Labels
contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) good first issue A good starting issue for contributors (issues with this label will appear in /contribute) package:native_toolchain_c

Comments

@dcharkes
Copy link
Collaborator

dcharkes commented Jul 12, 2024

The CBuilder lists the C sources, but not the header files.

final cbuilder = CBuilder.library(
name: packageName,
assetName: '$packageName.dart',
sources: [
'src/$packageName.c',
],
dartBuildFiles: ['hook/build.dart'],
);

This means the headers are not added to the dependencies. Which means that any changes to the header files do not cause cache invalidation.

Thanks for the feedback @SaltySpaghetti!

Possible solutions:

  1. Pass in the .h files to sources, which will add them to the compiler invocation (which is fine for clang-like compiler).
    • Pro: Clanglike compilers can optimize if we pass the .h files to the compilation step. https://gcc.gnu.org/onlinedocs/gcc/Precompiled-Headers.html
    • Not every compiler might like this. We can work around this by filtering .h files out.
    • Pro: Similar design to GN/ninja, which also have the c and h files in a flat list.
  2. Add the header directory to the includes (ty @blaugold!)
  3. Add a List<String> headers parameter/field. Which would only resolve the list of paths and add it to the BuildOutput dependencies.
    • Pro: most to the point.
    • Con: if there's other dependencies which are not header files this parameter can be used to shoe-horn these deps in.
  4. Add a List<String> dependencies parameter/field.
  • Con: Is not as discoverable as headers.

I'm leaning towards option 1.

A PR would contain:

  • Updating all example/test projects to include the header files in sources:
  • Adding some documentation to sources that it should include header files.
  • If needed for some compilers, filtering out .h files in the compiler invocation.
@dcharkes dcharkes added contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) package:native_toolchain_c good first issue A good starting issue for contributors (issues with this label will appear in /contribute) labels Jul 12, 2024
@blaugold
Copy link
Contributor

Just FYI, includes adds all files that are contained in the provided directories to dependencies. So you could just add the directory with the headers to includes.

/// Include directories to pass to the compiler.
///
/// Resolved against [BuildConfig.packageRoot].
///
/// Used to output the [BuildOutput.dependencies].
final List<String> includes;

@dcharkes
Copy link
Collaborator Author

Just FYI, includes adds all files that are contained in the provided directories to dependencies. So you could just add the directory with the headers to includes.

/// Include directories to pass to the compiler.
///
/// Resolved against [BuildConfig.packageRoot].
///
/// Used to output the [BuildOutput.dependencies].
final List<String> includes;

Thanks @blaugold!
If option 1 works though, I believe that option 1 is better. But if there's issues with option 1, than option 2 is a good one too.

@MiniPiku
Copy link

MiniPiku commented Mar 12, 2025

@dcharkes kindly take a look at the changes made
#2098
thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) good first issue A good starting issue for contributors (issues with this label will appear in /contribute) package:native_toolchain_c
Projects
None yet
Development

No branches or pull requests

3 participants