Repository navigation
Remove vestigial download_jdk gclient var - #188571
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the "download_jdk": false variable from the gclient_variables configuration across multiple CI builder JSON files for Linux, macOS, and Windows. There are no review comments, and I have no feedback to provide.
|
Two-person ping; this is a pure-deletion CL closing #187627.
No CODEOWNERS rule covers |
`download_jdk` was declared in DEPS:97 with default True but no DEPS entry ever referenced it as a condition. The only OpenJDK CIPD entry (`engine/src/flutter/third_party/java/openjdk`) gates only on host_os/host_cpu: 'condition': 'not (host_os == "linux" and host_cpu == "arm64")', So setting `download_jdk: false` in builder configs was a no-op -- OpenJDK was downloaded anyway on every host except linux-arm64 (which is correctly excluded by the host_cpu condition, not by download_jdk). This change: - Removes the unused `download_jdk` declaration from DEPS. - Removes `"download_jdk": false` from 22 builder JSONs across engine/src/flutter/ci/builders/. 108 occurrences in total. None of them did anything; their removal has no behavioral effect. The OpenJDK download condition is unchanged. Builds that don't actually need the JDK (e.g. linux_arm64_android_aot_engine) still get it on linux-x64 hosts -- if that becomes worth optimizing, the OpenJDK condition itself is the right place to add a gate, not a separate variable that doesn't propagate. Closes flutter#187627 @reidbaker noted on the issue: "If it is not used feel free to delete it" -- doing that.
13e894d to
d9216a4
Compare
|
ok I dug through the history. The variable was added so that we could avoid downloading java from cipd on ci machines. But I never came back and hooked up that variable to the cipd system. Jason made the ci machines more efficient about 2 months ago. Currently we download java if we are not on a arm46 linux machine (best I can tell) Lines 622 to 632 in 880b9c5 This pr removes unused code which seems good unless we should be using that unused code. I dont have strong feelings one way or another. |
|
I don't know how long it takes to download Java during caching steps, but the machines are faster now. If it is dead code, then let's trim it. I would like for us to have linux arm64 java in cipd at some point as well, but that's out of scope. |
|
Pulled real numbers before answering. On a recent Linux engine build (b/8676713848448018577):
The package itself is ~212 MB on the wire, pinned to a static So on the speed question: wiring |
|
Removal of dead code is good. The bots dont seem to need to optimization that new code could improve. Thanks all |
|
Thanks for the review @reidbaker! I don't have label permissions here — could you add |
|
Yeah the second reviewer will add auto submit we require 2 approvals |
|
autosubmit label was removed for flutter/flutter/188571, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
re running the failing dashboard checks in case they are flakes. |
|
@dbebawy - can you update the branch and resolve the conflicts? Sorry about the test flakes. |
|
I'll wait for @reidbaker to re-approve. I suspect he just needs to approve and add the autosubmit label. |
|
autosubmit label was removed for flutter/flutter/188571, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
re-running flakey test :( |
|
@jtmcdole @reidbaker friendly ping — this is still approved and merges cleanly, just stuck on the Google testing check from Sep 14. Could one of you re-run it and re-add |
|
I believe this PR might only need to be rebased to tip-of-tree. |
flutter/flutter@55b8f88...d649d2b 2026-09-30 43054281+camsim99@users.noreply.github.com [Android] Update the CLI to reject passing engine configuration flags with a prebuilt binary in release mode (flutter/flutter#190870) 2026-09-30 engine-flutter-autoroll@skia.org Roll Skia from af1e8b356dd1 to c7b323126bf0 (1 revision) (flutter/flutter#193569) 2026-09-30 zhongliu88889@gmail.com [web] Respect text affinity in getLineBoundary at a soft wrap (flutter/flutter#192664) 2026-09-30 bkonyi@google.com [tool] Migrate TestCommand and platform runner to modular dependency injection (flutter/flutter#190789) 2026-09-30 engine-flutter-autoroll@skia.org Roll Skia from 3a20a0464d25 to af1e8b356dd1 (1 revision) (flutter/flutter#193564) 2026-09-30 engine-flutter-autoroll@skia.org Roll Skia from 554ef62d11a9 to 3a20a0464d25 (6 revisions) (flutter/flutter#193556) 2026-09-30 116356835+AbdeMohlbi@users.noreply.github.com Use null aware elements in `platform_views.dart` (flutter/flutter#193214) 2026-09-30 116356835+AbdeMohlbi@users.noreply.github.com Replace deprecated `withOpacity` in `flutter_logo.dart` (flutter/flutter#192583) 2026-09-30 engine-flutter-autoroll@skia.org Roll Skia from f441ca223b2b to 554ef62d11a9 (72 revisions) (flutter/flutter#193548) 2026-09-30 joel.winarske@linux.com Fix StrcmpFixed matching prefixes in the Vulkan embedder tests (flutter/flutter#192999) 2026-09-30 jesswon@google.com Update Engine to test Android 17 AVD (flutter/flutter#193239) 2026-09-29 joel.winarske@linux.com Give the Vulkan test context the extensions Impeller requires (flutter/flutter#193002) 2026-09-29 55765052+MohanadAbdallah-mv@users.noreply.github.com Rename "subtext" to "supportingTextPadding" in `FormField`'s documentation (flutter/flutter#183582) 2026-09-29 markzipan@google.com Set --no-js-strongly-connected-components for DDC compiles by default (flutter/flutter#193485) 2026-09-29 jesswon@google.com Split pre-AGP 8.3 module AAR test into a Java 17 pinned target (flutter/flutter#193467) 2026-09-29 32538273+ValentinVignal@users.noreply.github.com Remove no-shuffle from gen_defaults_test (flutter/flutter#192280) 2026-09-29 katelovett@google.com Update guidance on bumping Dart (flutter/flutter#193131) 2026-09-29 dbebawy@users.noreply.github.com Remove vestigial `download_jdk` gclient var (flutter/flutter#188571) 2026-09-29 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#193522) 2026-09-29 bkonyi@google.com [flutter_tools] Lazily initialize AndroidSdk platform and build-tools discovery (flutter/flutter#191972) 2026-09-29 bkonyi@google.com [tool] Migrate Web build subcommands and toolchain to modular dependency injection (flutter/flutter#190783) 2026-09-29 matt.boetger@gmail.com Pin androidx.test dependencies in integration_test (flutter/flutter#193316) 2026-09-29 Deil.Christoph@gmail.com [flutter_tools] Include flutter.js.map in web builds (flutter/flutter#192257) 2026-09-29 Rusino@users.noreply.github.com [WebParagraph] Fixing edge cases for wrapping text (with newlines) (flutter/flutter#189858) 2026-09-29 ryjohn@google.com Bump customer testing version for flutter/devtools update (flutter/flutter#193511) 2026-09-29 engine-flutter-autoroll@skia.org Roll Packages from ba0364a to 3c6ce59 (14 revisions) (flutter/flutter#193507) 2026-09-29 bkonyi@google.com [flutter_tools] Guard socket streams and done futures against socket reset errors (flutter/flutter#192941) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC quncheng@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Follow-up to flutter#188571, which removed the `download_jdk` gclient var from DEPS and the builder JSONs under `ci/builders/`. Two references were outside that path and got missed: - `engine/src/flutter/.ci.yaml`: the Linux, Mac and Windows `builder_cache` targets still pass `"download_jdk": "true"`. DEPS no longer declares the var, so gclient drops the override and the line does nothing. - `lib/web_ui/dev/generate_builder_json.dart` still emits `'download_jdk': false`. On master today, `felt generate-builder-json` adds three `"download_jdk": false` lines back to `linux_web_engine_test.json`. With this change it regenerates the checked-in file with no diff. No behavior change. After this, `git grep download_jdk` is empty. Part of flutter#187627. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant in-code documentation (doc comments with `///`). - [ ] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Summary
Removes the vestigial
download_jdkgclient var. Closes #187627.download_jdkwas declared in DEPS:97 with defaultTruebut no DEPS entry ever referenced it as a condition. The only OpenJDK CIPD entry (engine/src/flutter/third_party/java/openjdk) gates only onhost_os/host_cpu:So setting
"download_jdk": falsein builder configs was a no-op — OpenJDK was downloaded anyway on every host except linux-arm64 (which is correctly excluded by the host_cpu condition, not bydownload_jdk).Changes
download_jdkdeclaration fromDEPS(lines 91-92, including the now-misleading comment "Checkout Java dependencies only on platforms that do not have java installed on path.")"download_jdk": falsefrom 22 builder JSONs inengine/src/flutter/ci/builders/. 108 occurrences total. None of them did anything; their removal has no behavioral effect.The OpenJDK download condition itself is unchanged. Builds that don't actually need the JDK (e.g.
linux_arm64_android_aot_engine,mac_clang_tidy) still get it on linux-x64/mac hosts — if that becomes worth optimizing, the OpenJDK condition itself is the right place to add a gate, not a separate variable that doesn't propagate.Context
Found while running validation builds for #187591. Filed #187627 noting
download_jdk: falsewas being ignored. @reidbaker on the issue:Doing that here.
Pre-launch Checklist
Test plan
host_os/host_cpu(unchanged); removing the unreferenceddownload_jdkvar and the no-op"download_jdk": falselines from builder configs cannot change what gets synced.python3 -c "import json; ..."confirms all 22 modified JSONs still parse.grep -rn download_jdk DEPS engine/src/flutter/ci/after the diff returns empty.