{{ message }}
Remove vestigial download_jdk gclient var#188571
Open
dbebawy wants to merge 1 commit into
Open
Conversation
`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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.