search

dart-lang / native

Dart packages related to FFI and native assets bundling.
BSD 3-Clause "New" or "Revised" License
155 stars 43 forks source link

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

Open dcharkes opened 3 months ago

dcharkes commented 3 months ago

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

https://github.com/dart-lang/native/blob/952da66a77c09872816227f5313273d51a8f4ba5/pkgs/native_assets_cli/example/build/native_add_library/hook/build.dart#L12-L19

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:

blaugold commented 3 months ago

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.

https://github.com/dart-lang/native/blob/7efefec16509ed38544a8af999b9e85694586bc7/pkgs/native_toolchain_c/lib/src/cbuilder/cbuilder.dart#L66-L71

dcharkes commented 3 months ago

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.

https://github.com/dart-lang/native/blob/7efefec16509ed38544a8af999b9e85694586bc7/pkgs/native_toolchain_c/lib/src/cbuilder/cbuilder.dart#L66-L71

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.