Add `package:record_use` with JSON storage #1479

mosuem · 2024-08-29T13:35:17Z

Update: Landing as https://dart-review.googlesource.com/c/sdk/+/383340 instead.

While this feature is under development, we should not over-complicate things and go with JSON as a storage format. Once we have a working feature behind an experimental flag, we should re-evaluate if other formats such as protobuf make more sense. Same goes for the API, which we should evolve while testing use cases.

This package will be imported into the SDK and used by the vm and dart2js.

After this is merged, import this package into g3, and then merge the corresponding SDK CL.

TODO: update https://dart-review.googlesource.com/c/sdk/+/369620 to this PR.

cc @dcharkes

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

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

github-actions · 2024-08-29T13:36:09Z

Package publishing

Package	Version	Status	Publish tag (post-merge)
package:record_use	0.1.0	ready to publish	`record_use-v0.1.0`
package:ffi	2.1.3	already published at pub.dev
package:ffigen	14.0.0-wip	WIP (no publish necessary)
package:jni	0.11.0	already published at pub.dev
package:jnigen	0.11.0	already published at pub.dev
package:native_assets_cli	0.7.4-wip	WIP (no publish necessary)
package:objective_c	2.0.0-wip	WIP (no publish necessary)
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

Does this superseed #1248 and #1117? Can those be closed?

(Refraining from reviewing the format and API.)

.github/workflows/record_use.yaml

pkgs/record_use/.github/workflows/test.yaml

pkgs/record_use/README.md

mosuem · 2024-08-30T07:23:53Z

Does this superseed #1248 and #1117? Can those be closed?

Yes and maybe - we still need a PR to expose the recorded usages to link hooks. But I might just refile one.

pkgs/record_use/lib/src/data_classes/annotation.dart

pkgs/record_use/lib/src/data_classes/arguments.dart

pkgs/record_use/lib/record_use_internal.dart

github-actions · 2024-09-03T08:06:18Z

PR Health

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/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:record_use	0.1.0	ready to publish
package:ffi	2.1.3	already published at pub.dev
package:ffigen	14.0.0-wip	WIP (no publish necessary)
package:jni	0.11.0	already published at pub.dev
package:jnigen	0.11.0	already published at pub.dev
package:native_assets_cli	0.7.4-wip	WIP (no publish necessary)
package:objective_c	2.0.0-wip	WIP (no publish necessary)
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.

mkustermann

I think this is going to need a number of iterations, but let's start landing & iterating.

lgtm with comments

pkgs/record_use/README.md

pkgs/record_use/lib/record_use_internal.dart

pkgs/record_use/README.md

pkgs/record_use/lib/record_use_internal.dart

pkgs/record_use/lib/src/record_use.dart

coveralls · 2024-09-03T10:08:41Z

coverage: 91.884%. remained the same
when pulling 728fb3d on addPackageRecordUseJson
into c329cf9 on main.

mkustermann · 2024-09-03T10:08:52Z

Actually, for our iteration speed and also conceptually, it may actually make sense for this package:record_use to live in the Dart SDK (i.e. dart-lang/sdk@pkg/record_use) for now: The code that generates the resource use json is in the Dart SDK (part of our compiler) and this package is the reading part of that format. So it'd be easier to iterate by having it in the Dart SDK.

Once the format is stable we could move it here (if there's a reason for it)

@mosuem wdyt?

mosuem · 2024-09-03T11:08:33Z

But we would still publish it, right?

Also, do we have any restrictions on adding packages to the SDK? I don't want to start any discussions there.

mkustermann · 2024-09-03T11:20:33Z

But we would still publish it, right?

Of course we can publish it.

Also, do we have any restrictions on adding packages to the SDK? I don't want to start any discussions there.

Not AFAIK

.github/workflows/record_use.yaml

This package provides (de-)serialization for recorded usages (formerly known as resource identifiers). Adding to the SDK packages for faster development for now - to be moved to dart-lang/ on Github in the future. See also the review at dart-lang/native#1479. Change-Id: I4796bbf5616f64ce700a1bc59f279883acb36263 Cq-Include-Trybots: luci.dart.try:pkg-linux-debug-try,pkg-linux-release-arm64-try,pkg-linux-release-try,pkg-mac-release-arm64-try,pkg-mac-release-try,pkg-win-release-arm64-try,pkg-win-release-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/383340 Commit-Queue: Moritz Sümmermann <[email protected]> Reviewed-by: Martin Kustermann <[email protected]>

mosuem · 2024-09-06T13:14:49Z

Closing for https://dart-review.googlesource.com/c/sdk/+/383340

mosuem added 3 commits August 29, 2024 14:49

Initial commit

0d48580

Add JSON based implementation

e036dae

Some fixes

3d1f6e6

mosuem requested a review from mkustermann August 29, 2024 13:35

github-actions bot added the type-infra A repository infrastructure change or enhancement label Aug 29, 2024

dcharkes reviewed Aug 29, 2024

View reviewed changes

.github/workflows/record_use.yaml Show resolved Hide resolved

.github/workflows/record_use.yaml Outdated Show resolved Hide resolved

pkgs/record_use/.github/workflows/test.yaml Outdated Show resolved Hide resolved

pkgs/record_use/README.md Outdated Show resolved Hide resolved

Address comments

ad99f0c

fix typo

b1477d4

mosuem mentioned this pull request Aug 30, 2024

Add package:record_use with protobuf storage #1248

Closed

1 task

mkustermann reviewed Sep 2, 2024

View reviewed changes

mosuem added 2 commits September 3, 2024 09:49

Some changes

2d3e8c1

Switch to equatable

85afe4f

mosuem requested a review from mkustermann September 3, 2024 08:03

Add record_use to health checks

65ef88e

mosuem added the skip-breaking-check label Sep 3, 2024

mkustermann approved these changes Sep 3, 2024

View reviewed changes

Address comments

1cb52f2

github-actions bot added the package:native_assets_cli label Sep 3, 2024

Sort imports

27f7095

mosuem added 2 commits September 3, 2024 13:28

Restructure classname of instances

728fb3d

Undo native_assets_cli changes

621088c

github-actions bot removed the package:native_assets_cli label Sep 3, 2024

dcharkes reviewed Sep 3, 2024

View reviewed changes

.github/workflows/record_use.yaml Show resolved Hide resolved

dcharkes reviewed Sep 3, 2024

View reviewed changes

.github/workflows/record_use.yaml Show resolved Hide resolved

Remove equatable

da6d5db

mosuem marked this pull request as draft September 3, 2024 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `package:record_use` with JSON storage #1479

Add `package:record_use` with JSON storage #1479

mosuem commented Aug 29, 2024 •

edited

Loading

github-actions bot commented Aug 29, 2024 •

edited

Loading

dcharkes left a comment

mosuem commented Aug 30, 2024

github-actions bot commented Sep 3, 2024 •

edited

Loading

mkustermann left a comment

coveralls commented Sep 3, 2024 •

edited

Loading

mkustermann commented Sep 3, 2024 •

edited

Loading

mosuem commented Sep 3, 2024

mkustermann commented Sep 3, 2024

mosuem commented Sep 6, 2024

Add package:record_use with JSON storage #1479

Are you sure you want to change the base?

Add package:record_use with JSON storage #1479

Conversation

mosuem commented Aug 29, 2024 • edited Loading

Update: Landing as https://dart-review.googlesource.com/c/sdk/+/383340 instead.

github-actions bot commented Aug 29, 2024 • edited Loading

Package publishing

dcharkes left a comment

Choose a reason for hiding this comment

mosuem commented Aug 30, 2024

github-actions bot commented Sep 3, 2024 • edited Loading

PR Health

mkustermann left a comment

Choose a reason for hiding this comment

coveralls commented Sep 3, 2024 • edited Loading

mkustermann commented Sep 3, 2024 • edited Loading

mosuem commented Sep 3, 2024

mkustermann commented Sep 3, 2024

mosuem commented Sep 6, 2024

Add `package:record_use` with JSON storage #1479

Add `package:record_use` with JSON storage #1479

mosuem commented Aug 29, 2024 •

edited

Loading

github-actions bot commented Aug 29, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading

coveralls commented Sep 3, 2024 •

edited

Loading

mkustermann commented Sep 3, 2024 •

edited

Loading