Skip to content
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

[native_assets_builder] Set the last modified date of assets to the build output timestamp #1533

Closed
wants to merge 1 commit into from

Conversation

blaugold
Copy link
Contributor

@blaugold blaugold commented Sep 9, 2024

With this PR, all assets that are produced by a hook have their last modified date set to the HookOutput.timestamp.

This ensures that caching mechanisms can use the last modified time of assets to invalidate cached results that depend on assets. See https://dart-review.googlesource.com/c/sdk/+/381580 for context.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@github-actions github-actions bot added type-infra A repository infrastructure change or enhancement package:native_assets_builder labels Sep 9, 2024
@blaugold blaugold changed the title [native_assets_builder] Set the last modified date of assets to the build output timestamp [native_assets_builder] Set the last modified date of assets to the build output timestamp Sep 9, 2024
Copy link

github-actions bot commented Sep 9, 2024

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
Changelog Entry ✔️
Package Changed Files

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.

Package Leaked API symbols
License Headers ✔️
// 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
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/ffigen/example/libclang-example/generated_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/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_enum_int_mimic_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/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/bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/swift2objc/lib/src/config.dart
pkgs/swift2objc/lib/src/generate_wrapper.dart
pkgs/swift2objc/lib/src/generator/_core/utils.dart
pkgs/swift2objc/lib/src/generator/generator.dart
pkgs/swift2objc/lib/src/generator/generators/class_generator.dart
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_property_declaration.dart
pkgs/swift2objc/lib/src/transformer/_core/unique_namer.dart
pkgs/swift2objc/lib/src/transformer/_core/utils.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_property.dart
Package publish validation ✔️
Package Version Status
package:ffi 2.1.3 already published at pub.dev
package:ffigen 14.0.0 already published at pub.dev
package:jni 0.11.0 already published at pub.dev
package:jnigen 0.11.1-wip WIP (no publish necessary)
package:native_assets_cli 0.8.0-wip WIP (no publish necessary)
package:objective_c 2.0.0 already published at pub.dev
package:swift2objc 0.0.1-wip WIP (no publish necessary)
package:swiftgen 0.0.1-wip WIP (no publish necessary)

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

@dcharkes
Copy link
Collaborator

dcharkes commented Sep 9, 2024

We need to think a bit more about this.

Changing the date only works if the output file is in the output directory. If we have assets/asset.txt and for all architectures/OSes we don't copy the file into the output directory, then every hook (even concurrently) will try to set the last modified timestamp of the source file.

If building assets can be shared for some configurations (let's say debug/release doesn't make a difference), then we'd want to encourage caching such asset in .dart_tool/my_package/assets/... (#1375).

And for in-hook caching, I'd like to give hook writers the contract that the hook runner does not modify any files.

Also, if the hook runs because 1 asset changed, but 10 assets didn't we'll touch the other 10 assets and have to recopy them.

Some options:

  • So, we either force copying assets into the output dir, and don't promise we don't edit but might touch those files. And only copy if the timestamp changed.
  • Or, we force copying those files in Dart/Flutter (as in https://dart-review.googlesource.com/c/sdk/+/381580) to the same dir for every invocation.
  • Or, we force copying the asset files in Dart/Flutter to a temp dir for every invocation.
  • (And the last two could be augmented by saving hashes and only doing copying/resigning if the hash is changed.)

Maybe the deeper question is what concurrency we want to support:

  • Running 2x a Dart program, not modifying the hook/assets.
  • Running a Dart program that keeps running, modifying the hook, and starting the same program again! This would need to keep a lock on the build hook until the assets are copied to a temp dir, and only then can the hook be released. Otherwise, while running other hooks in the first invocation of the Dart program, the second Dart program can try to delete and recreate the asset files.
  • ... ?

This last use case is currently broken.

I think we can only support the last use case if we already copy in native_assets_builder before returning even dartdev/flutter_tools. (Or we need to release the directory locks much later.) And if that is a different temp dir every time, we should pass in that temp dir to the NativeAssetsBuildRunner constructor so that flutter_tools and dartdev can create it and delete it.

We can still try to avoid extra copies/resigns hashing the file contents, but I'm not sure if that's faster or slower.

Sorry to send you in to the weeds @blaugold. I'm discovering requirements as we go! 🤓 Any other concurrency cases you can think of, now that we're at it?

@blaugold
Copy link
Contributor Author

Sorry to send you in to the weeds @blaugold. I'm discovering requirements as we go! 🤓 Any other concurrency cases you can think of, now that we're at it?

No problem, that's how it goes. 🙂 And thanks for taking the time to write down your thoughts!

I can't think of other concurrency scenarios, ATM.

@dcharkes
Copy link
Collaborator

With #1750, the native assets builder will use file content hashing.

So, we'll no longer have to worry about timestamps.

@dcharkes dcharkes closed this Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:native_assets_builder type-infra A repository infrastructure change or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants