From da40aec0bb375876c2f3b8dd7f2fb0cc53d00125 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Wed, 27 May 2026 07:31:46 -0700 Subject: [PATCH] [deps] Roll dart-lang/native Pass in the entry-points and OS to the link hook when the record_use file is provided. So that different target OSes and different entry- points are cached separately. Also, commit skill to rev `native_rev` in DEPS. (The new agent IDE ignores gemini.md that contained it and prefers using skills.) Change-Id: Ic77e3ccd5915736081c01f8b75901f626f688f19 Cq-Include-Trybots: luci.dart.try:dart2wasm-asserts-linux-chrome-try,dart2wasm-asserts-minified-linux-d8-try,dart2wasm-linux-chrome-try,dart2wasm-linux-d8-try,dart2wasm-linux-firefox-try,dart2wasm-linux-jscm-chrome-try,dart2wasm-linux-optimized-jsc-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,dart2js-canary-linux-try,dart2js-hostasserts-linux-d8-try,dart2js-linux-chrome-try,dart2js-linux-firefox-try,dart2js-mac-chrome-try,dart2js-mac-safari-try,dart2js-minified-csp-linux-chrome-try,dart2js-minified-linux-d8-try,dart2js-unit-linux-x64-release-try,dart2js-win-chrome-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/506580 Commit-Queue: Daco Harkes Reviewed-by: Michael Goderbauer --- .agents/skills/update_native_rev/SKILL.md | 105 ++++++++++++++++++ DEPS | 2 +- pkg/dartdev/lib/src/commands/build.dart | 13 ++- pkg/dartdev/lib/src/native_assets.dart | 21 +++- .../test/native_assets/build_test.dart | 65 +++++++++++ 5 files changed, 202 insertions(+), 4 deletions(-) create mode 100644 .agents/skills/update_native_rev/SKILL.md diff --git a/.agents/skills/update_native_rev/SKILL.md b/.agents/skills/update_native_rev/SKILL.md new file mode 100644 index 00000000000..c3fdd679819 --- /dev/null +++ b/.agents/skills/update_native_rev/SKILL.md @@ -0,0 +1,105 @@ +--- +name: update-native-rev +description: Updates the native_rev dependency in the DEPS file with all correct verification, sync, and testing procedures +--- + + + +# Instructions + +Use this skill when you are requested to roll or update the `native_rev` variable (the hash of `third_party/pkg/native` repository) in the `DEPS` file. + +## 1. Commit Selection Rules + +> [!IMPORTANT] +> **CRITICAL RULE**: NEVER EVER pick a local commit or a commit from a custom branch/PR branch. You MUST only pick remote commits that have already landed on the remote `main` branch. + +To select the correct commit SHA: +1. **Ask the user** if the newest hash/commit should be taken directly from **GitHub** (e.g., if the googlesource mirror is lagging behind or the desired change is only on GitHub main). + * **If the user says YES**: + 1. First, temporarily change the repository URL and host permissions in the `DEPS` file by following the instructions in the **[Troubleshooting Sync Failures](#troubleshooting-sync-failures)** section (changing `third_party/pkg/native` under `deps` to use `"https://github.com/dart-lang/native.git"` and adding `'github.com'` to `allowed_hosts` in `DEPS`). + 2. Run `gclient sync` from the SDK root to update the checkout mapping to GitHub. + 3. Navigate to the `native` package subdirectory: `third_party/pkg/native`. + 4. Run `git fetch origin` to fetch the latest commits from the GitHub remote (origin will now point to GitHub). + 5. After checking commits, **immediately set the repository URL and host permissions in `DEPS` back to googlesource** (reverting the changes made in sub-step 1), so that subsequent steps and the final commit only roll the commit hash without permanently keeping GitHub host settings. + * **If the user says NO / default**: + 1. Proceed with the default mirrored googlesource setup. + 2. Navigate to the `native` package subdirectory: `third_party/pkg/native`. + 3. Run `git fetch origin` to fetch the latest commits from the mirror. +2. Inspect the commits on remote `origin/main` of the `native` repository: + ```bash + git log origin/main -n 10 --oneline + ``` +3. Obtain the full SHA of the newest commit on remote `origin/main` (or the specific one that contains the required change): + ```bash + git rev-parse origin/main + ``` +4. **Revert Host to GoogleSource**: + * **IMPORTANT**: If the host was set to GitHub, you **MUST** now revert the repository URL and allowed hosts changes in the `DEPS` file back to the mirrored googlesource URL, removing `github.com` from `allowed_hosts`. Do this before updating `native_rev` so that the final committed change only rolls the commit hash, not checkout hosts. + +## 2. Updating DEPS and Synchronizing + +1. Locate `native_rev` in the `DEPS` file (typically under `vars`): + ```python + "native_rev": "", # rolled manually while record_use is experimental + ``` +2. Replace the hash with the chosen remote `origin/main` commit SHA. +3. If not already done in step 4 above, ensure the `DEPS` host and URL configuration are reverted to the googlesource mirror, leaving only the updated `native_rev` hash. +4. Run `gclient sync -f` from the SDK root directory to update the checkout of the dependencies and regenerate the package configuration using the mirror. + +### Troubleshooting Sync Failures +If the synchronization fails because of host restrictions: +1. Temporarily modify the repository configuration under `deps` in `DEPS` to use: + `https://github.com/dart-lang/native.git` as the git repo. +2. Temporarily add `'github.com'` to the `allowed_hosts` list in `DEPS`. +3. Re-run `gclient sync -f` to complete the update. +4. **IMPORTANT**: Immediately after successful syncing, revert the host and URL changes in the `DEPS` file, removing `github.com` from `allowed_hosts` and returning to the mirrored googlesource URL, but keep the newly synced native commit hash. + +## 3. Mandatory Testing and Verification + +Before completing the update, you must run the following tests: + +### Native Assets Package Tests +Build the target SDK and run the primary native asset tests: +```bash +tools/build.py -mrelease create_sdk runtime ffi_test_functions runtime_precompiled && tools/test.py -n unittest-asserts-release-mac-arm64 pkg/dartdev/test/native_assets/ +``` + +### Record Use Optimization Tests +Since `record_use` integrates closely with `native_rev` updates, these tests must be validated for both backends (Wasm and VM). + +1. Build the required target backends and run all record use tests: + ```bash + tools/build.py -mrelease create_sdk dart2wasm runtime ffi_test_functions runtime_precompiled && xcodebuild/ReleaseARM64/dart-sdk/bin/dart pkg/compiler/test/record_use/record_use_test.dart && xcodebuild/ReleaseARM64/dart-sdk/bin/dart pkg/dart2wasm/test/record_use_test.dart && xcodebuild/ReleaseARM64/dart-sdk/bin/dart pkg/vm/test/transformations/record_use_test.dart + ``` +2. **Updating Expectations**: + > [!IMPORTANT] + > If the record_use tests fail, **DO NOT** update the expectations automatically. You **MUST** first ask the user for confirmation/permission and clarify if updating expectations is the correct course of action. + + If the user confirms that the expectations should be updated to match the new behavior: + * Run the test command with `-DupdateExpectations=true`: + ```bash + tools/build.py -mrelease create_sdk dart2wasm runtime ffi_test_functions runtime_precompiled && xcodebuild/ReleaseARM64/dart-sdk/bin/dart pkg/compiler/test/record_use/record_use_test.dart -DupdateExpectations=true && xcodebuild/ReleaseARM64/dart-sdk/bin/dart pkg/dart2wasm/test/record_use_test.dart -DupdateExpectations=true && xcodebuild/ReleaseARM64/dart-sdk/bin/dart pkg/vm/test/transformations/record_use_test.dart -DupdateExpectations=true + ``` + * **Note**: Always run the VM test one last when updating expectations. The expect files are stored in the VM directory. +3. **Running Individual Tests**: + To debug or verify a single test: + ```bash + python3 tools/test.py -n wasm-unittest-asserts-mac pkg/dart2wasm/test/record_use_test.dart + ``` + +## 4. Pre-Completion Protocol + +Follow these final validation steps before announcing completion: + +1. **Format touched files**: Run `dart format` on all modified Dart files. +2. **Analyze touched files**: Run `dart analyze` to ensure there are no lint or static analysis issues. +3. **Presubmit checks**: Run `git cl presubmit` to ensure build/format sanity. +4. **Update Coverage (Front-End changes only)**: + If any related changes were also introduced in the `front_end` directory, regenerate test coverage: + ```bash + dart --enable-asserts pkg/front_end/test/coverage_suite.dart --tasks=5 --add-and-remove-comments + ``` + * *Critical*: Revert the coverage comment changes in any files that you did not otherwise modify. diff --git a/DEPS b/DEPS index b3556866586..1d2b03c76d2 100644 --- a/DEPS +++ b/DEPS @@ -143,7 +143,7 @@ vars = { "i18n_rev": "2ae32fdd9ca14ecdca77dc37787dbba3c9e2c48a", "leak_tracker_rev": "f5620600a5ce1c44f65ddaa02001e200b096e14c", # rolled manually "material_color_utilities_rev": "799b6ba2f3f1c28c67cc7e0b4f18e0c7d7f3c03e", - "native_rev": "77bd486568a6989a5c0a1c896793e99a21e71df7", # rolled manually while record_use is experimental + "native_rev": "b814f5393753e0cd752ce3ad733f5e66dd5949ce", # rolled manually while record_use is experimental "protobuf_rev": "84079e8b8531309e06ba7276b1c28bdca9210ad6", "pub_rev": "ec276d10a7fa0f6c6ec005340fb9ad29f3b012d0", # rolled manually "shelf_rev": "74d58bc1499115f91f8120dbd08f77c83f7f343d", diff --git a/pkg/dartdev/lib/src/commands/build.dart b/pkg/dartdev/lib/src/commands/build.dart index dcb285df9fd..2a9ee3ec944 100644 --- a/pkg/dartdev/lib/src/commands/build.dart +++ b/pkg/dartdev/lib/src/commands/build.dart @@ -321,7 +321,13 @@ then that is used instead.''', for (final e in executables) { String? recordedUsagesPath; if (recordUseEnabled) { - recordedUsagesPath = path.join(tempDir.path, 'recorded_usages.json'); + final stableDirUri = + pubspecUri?.resolve('.dart_tool/native_assets/') ?? tempDir.uri; + final stableDir = Directory.fromUri(stableDirUri); + await stableDir.create(recursive: true); + recordedUsagesPath = stableDir.uri + .resolve('recorded_usages.json') + .toFilePath(); } final outputExeUri = binDirectory.uri.resolve( targetOS.executableFileName(e.name), @@ -353,17 +359,22 @@ then that is used instead.''', // Multiple executables are only supported with recorded uses // disabled, so don't re-invoke link hooks. if (hasHooks) { + final entryPoints = executables + .map((e) => e.sourceEntryPoint) + .toList(); linkResult = await (showProgress ? progress( 'Running link hooks', () => builder.linkNativeAssetsAOT( recordedUsagesPath: recordedUsagesPath, + entryPoints: entryPoints, buildResult: buildResult!, ), progressUpdatesOnStderr: progressUpdatesOnStderr, ) : builder.linkNativeAssetsAOT( recordedUsagesPath: recordedUsagesPath, + entryPoints: entryPoints, buildResult: buildResult!, )); if (linkResult == null) { diff --git a/pkg/dartdev/lib/src/native_assets.dart b/pkg/dartdev/lib/src/native_assets.dart index d13e51be23d..06732148776 100644 --- a/pkg/dartdev/lib/src/native_assets.dart +++ b/pkg/dartdev/lib/src/native_assets.dart @@ -191,13 +191,30 @@ class DartNativeAssetsBuilder { Future linkNativeAssetsAOT({ required String? recordedUsagesPath, + required List entryPoints, required BuildResult buildResult, }) async { final builder = await _nativeAssetsBuildRunner; final linkResult = await builder.link( extensions: _extensions, - resourceIdentifiers: recordedUsagesPath != null - ? Uri.file(recordedUsagesPath) + recordUse: recordedUsagesPath != null + ? RecordUseConfig( + file: Uri.file(recordedUsagesPath), + entryPoints: entryPoints, + // The compiler inlines target operating system constants before Type + // Flow Analysis (TFA) (see VMConstantEvaluator class and its + // visitStaticGet method inside + // pkg/vm/lib/transformations/vm_constant_evaluator.dart), which + // alters the recorded usages (tree-shaking). + // Thus, we must separate the link cache by target OS. Other build targets + // (like architecture and sanitizers) do not affect this optimization. + // + // We also use a stable 'vm_aot' prefix rather than the dynamic SDK + // compiler version string, ensuring old cache directories under + // `.dart_tool/` are overwritten/re-used rather than accumulating + // indefinitely across SDK rolls. + compiler: 'vm_aot_${target.os.name}', + ) : null, buildResult: buildResult, ); diff --git a/pkg/dartdev/test/native_assets/build_test.dart b/pkg/dartdev/test/native_assets/build_test.dart index 0e002644cfb..9a0fb334b87 100644 --- a/pkg/dartdev/test/native_assets/build_test.dart +++ b/pkg/dartdev/test/native_assets/build_test.dart @@ -277,6 +277,71 @@ void main(List args) { }); } + test('dart build link hook cache isolation', timeout: longTimeout, () async { + await recordUseTest('drop_dylib_recording', (dartAppUri) async { + // First run: compile with target drop_dylib_recording_calls.dart. + // This is the first compile, so it should run both build and link hooks. + final run1 = await runDart( + arguments: [ + '--enable-experiment=record-use', + 'build', + 'cli', + '--target', + 'bin/drop_dylib_recording_calls.dart', + '-v', + ], + workingDirectory: dartAppUri, + logger: logger, + expectExitCodeZero: true, + ); + expect(run1.stdout, contains('Running build hooks')); + expect(run1.stdout, contains('Running link hooks')); + expect(run1.stdout, isNot(contains('Skipping build for'))); + expect(run1.stdout, isNot(contains('Skipping link for'))); + + // Second run: compile with target drop_dylib_recording_calls.dart again. + // Since no inputs changed, it should skip both build and link hooks (cache hit). + final run2 = await runDart( + arguments: [ + '--enable-experiment=record-use', + 'build', + 'cli', + '--target', + 'bin/drop_dylib_recording_calls.dart', + '-v', + ], + workingDirectory: dartAppUri, + logger: logger, + expectExitCodeZero: true, + ); + expect(run2.stdout, contains('Skipping build for drop_dylib_recording')); + expect(run2.stdout, contains('Skipping link for drop_dylib_recording')); + expect(run2.stdout, isNot(contains('hook.dill'))); + + // Third run: compile with target drop_dylib_recording_instances.dart. + // The entrypoint target changed. + // The build hook is NOT dependent on entrypoints, so build should remain a cache hit. + // The link hook is dependent on entrypoints, so link must cache miss and run again. + final run3 = await runDart( + arguments: [ + '--enable-experiment=record-use', + 'build', + 'cli', + '--target', + 'bin/drop_dylib_recording_instances.dart', + '-v', + ], + workingDirectory: dartAppUri, + logger: logger, + expectExitCodeZero: true, + ); + expect(run3.stdout, contains('Skipping build for drop_dylib_recording')); + expect(run3.stdout, + isNot(contains('Skipping link for drop_dylib_recording'))); + expect(run3.stdout, contains('hook.dill')); + }); + }); + test( 'dart build with native dynamic linking', timeout: longTimeout,