[ffigen] ffigen only regenerates if input mtimes > output mtimes (#874)

[Abdk4Moura]

Before this, ffigen generates and overwrites any previously generated dart bindings. Now, it checks if any of the input files has been updated (including the config file itself) and only regenerates if the input files have indeed been regenerated.

[google-cla]

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.

[dcharkes]

@Abdk4Moura Thanks!

Please add tests. The tests should also cover situations where directories and glob patterns are used instead of only specific files.

[dcharkes]

typo

[Abdk4Moura]

Thanks! just corrected the typo

[Abdk4Moura]

I have added tests for the _inputHasBeenUpdated function.

[Abdk4Moura]

@dcharkes
But does it make a difference with globs and normal file names. Globs, as they seem to me, must have already been resolved by either the Config config or Library library objects.

Is a test with respect to those still relevant?

[github-actions]

PR Health

Breaking changes ✔️

Details

Package	Change	Current Version	New Version	Needed Version	Looking good?
ffigen	None	12.0.0-wip	12.0.1	12.0.1	✔️

Changelog Entry ✔️

Details

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️

Details

File	Coverage
pkgs/ffigen/lib/src/executables/ffigen.dart	💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check

License Headers ⚠️

Details

// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
pkgs/ffigen/test/code_generator_tests/ffigen_smart_regen/generated_bindings.dart

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/ffi/example/main.dart
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/example/objective_c/avf_audio_bindings.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/example/swift/swift_api_bindings.dart
pkgs/ffigen/lib/src/config_provider/config_spec.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/lang/jcharacter.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/_init.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/c_based/dart_bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/test/jackson_core_test/third_party/dart_only/dart_bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart

This check can be disabled by tagging the PR with skip-license-check

Package publish validation ❗

Details

Package	Version	Status
package:ffi	2.1.2	already published at pub.dev
package:jni	0.8.0-wip	WIP (no publish necessary)
package:jnigen	0.8.0-wip	WIP (no publish necessary)
package:native_assets_builder	0.5.0	already published at pub.dev
package:native_assets_cli	0.4.3-wip	WIP (no publish necessary)
package:ffigen	12.0.1	(error) pub publish dry-run failed; add the publish-ignore-warnings label to ignore

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

This check can be disabled by tagging the PR with skip-version-check

[coveralls]

coverage: 92.59% (-0.6%) from 93.216%
when pulling 9ac2fd6 on Abdk4Moura:main
into 001910c on dart-lang:main.

[dcharkes]

Sorry for the delay, I have too many tasks at the moment.

Any chance @liamappelbe and @mannprerak2 can review this instead?

[mannprerak2]

No problem @dcharkes :D

@Abdk4Moura this only considers the last modified times for entrypoints headers declared in config. But it's possible some other imported header file might have changed instead.

Any transitive dependencies could be changed and will be missed by this.

Some more edge cases to consider -

1. ffigen version might be updated in pubspec.yaml
2. Any updates to compiler-opts specified via the environment itself.

[liamappelbe]

I'm hesitant to add this at all. In my experience with rolling my own build system, it's actually surprisingly difficult to catch all the edge cases where deps have been modified. It would be really frustrating for our users, and difficult for them to diagnose, if we missed one of those edge cases, and ffigen was just not running.

My preference would be to let a higher level of build system handle checking the file times, like Bazel or package:build or native assets.

If people really want this, then we at least need a command line flag and/or a config option to force rebuild.

[Abdk4Moura]

I was thinking of that too; a "--force" flag

[dcharkes]

Thanks @mannprerak2 and @liamappelbe!

Based on your comments, some high level observations.

My preference would be to let a higher level of build system handle checking the file times, like Bazel or package:build or native assets.

👍 Great thinking about how this integrates in the larger eco system.

I think the FFIgen (and JNIgen) should be able to report the list of dependencies (#1008). We don't want every user of FFIgen (and JNIgen) to manually come up with a list of dependencies.

The logic of checking the files timestamps could maybe live in a shared package that then FFIgen, JNIgen, and native_assets_builder can reuse.

One question to ask is what abstraction to use for listing dependencies:

1. File paths
2. Directory paths (which would mean all files transitively in that directory, and it also means that if someone adds a file in such directory it's added).
3. Glob patterns? (which would mean that any files added to the file system matching such pattern would trigger a rebuild). -> But not every build system wrapping FFIgen/JNIgen might be able consume glob patterns (native assets, package:build, ...)

[dcharkes]

I have distilled some of the learnings from the PR into #1101.

Thanks @Abdk4Moura for the suggestion and the exploration!

I don't believe we can land this PR in it's current state, I'll close it.