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.

Sign up for GitHub

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

Jump to bottom

[native_assets_builder] Don't cache hook invocations if env vars change #1759

Merged
merged 3 commits into from
Dec 4, 2024
Merged

[native_assets_builder] Don't cache hook invocations if env vars change #1759

merged 3 commits into from
Dec 4, 2024

Conversation

dcharkes
Copy link
Collaborator

@dcharkes dcharkes commented Nov 27, 2024 •
edited
Loading

This PR makes the native assets builder save the Platform.environment a hook is run in. Subsequent invocations check if the environment didn't change.

This removes the includeParentEnvironment parameter everywhere. It was always set to true in dartdev and flutter_tools (This will require a manual roll into the Dart SDK and Flutter tools.)

A follow up PR should restrict the list of environment variables (#1764), this PR is about caching correctness.

Bug:

@github-actions GitHub Actions
Copy link

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_sort_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/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/bindings/printer.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/test/type_path_test.dart
pkgs/jnigen/tool/command_runner.dart
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart
pkgs/objective_c/lib/src/ns_input_stream.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/parser/parsers/declaration_parsers/parse_initializer_declaration.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_globals.dart
pkgs/swift2objc/lib/src/transformer/transformers/transform_variable.dart

@dcharkes dcharkes marked this pull request as ready for review November 27, 2024 19:49
@coveralls
Copy link

coveralls commented Nov 27, 2024 •
edited
Loading

Coverage Status

coverage: 88.922% (+0.07%) from 88.857%
when pulling 4814ef9 on env-vars
into 9c33168 on main.

@dcharkes
Copy link
Collaborator Author

@HosseinYousefi I don't know how to exclude the parent PR from the diff, but you could simply look at the last commit.

@dcharkes dcharkes requested a review from mkustermann November 28, 2024 08:43
@dcharkes dcharkes mentioned this pull request Nov 28, 2024
Merged
@dcharkes dcharkes requested a review from mkustermann November 28, 2024 14:23
@dcharkes dcharkes mentioned this pull request Nov 28, 2024
Closed
mkustermann
mkustermann reviewed Nov 29, 2024
View reviewed changes
Copy link
Member

@mkustermann mkustermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With comments addressed: lgtm

pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart Outdated Show resolved Hide resolved
@dcharkes dcharkes requested a review from mkustermann December 3, 2024 14:22
@dcharkes
Copy link
Collaborator Author

dcharkes commented Dec 3, 2024

With comments addressed: lgtm

@mkustermann We need an actual +1 to merge. 😄

mkustermann
mkustermann approved these changes Dec 3, 2024
View reviewed changes
Copy link
Member

@mkustermann mkustermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with comment

pkgs/native_assets_builder/lib/src/dependencies_hash_file/dependencies_hash_file.dart Outdated Show resolved Hide resolved
pkgs/native_assets_builder/lib/src/dependencies_hash_file/dependencies_hash_file.dart Show resolved Hide resolved
pkgs/native_assets_builder/lib/src/dependencies_hash_file/dependencies_hash_file.dart Show resolved Hide resolved
@auto-submit auto-submit bot merged commit d56a5b5 into main Dec 4, 2024
38 checks passed
@auto-submit auto-submit bot deleted the env-vars branch December 4, 2024 10:02
auto-submit bot pushed a commit that referenced this pull request Dec 4, 2024
@dcharkes
[native_assets_builder] Don't pass in the whole environment (#1764)
7e9897b 
Closes: #32

See the referenced issue for a reasoning on the list of environment variables.

Stacked on top of:

* #1759
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkustermann mkustermann mkustermann approved these changes

@HosseinYousefi HosseinYousefi Awaiting requested review from HosseinYousefi

Assignees
No one assigned
Labels
autosubmit package:native_assets_builder package:native_toolchain_c
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dcharkes @coveralls @mkustermann