Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds command-line flags, including --packages-file and --dry-run, to create_cipd_packages.sh, allows underscores in version tags, and introduces a preview_packages.txt file. It also adds a check to verify if a CIPD tag is already in use before downloading packages. The review feedback recommends skipping the packages file validation when the list command is used to prevent a regression, and bypassing the CIPD tag check during a dry run to avoid unnecessary network and credential requirements.
| # Validate the packages file exists. Relative paths are resolved against the | ||
| # current directory first, then against the directory of this script. | ||
| if [[ ! -f "$packages_file" && -f "$script_dir/$packages_file" ]]; then | ||
| packages_file="$script_dir/$packages_file" | ||
| fi | ||
| if [[ ! -f "$packages_file" ]]; then | ||
| echo "Packages file '$packages_file' not found." | ||
| print_usage | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The validation of the packages file is executed before checking if the command is list. Since the list command only queries the available packages from sdkmanager and does not use the packages file, requiring it to exist is a regression. We should only validate the packages file if the command is not list.
| # Validate the packages file exists. Relative paths are resolved against the | |
| # current directory first, then against the directory of this script. | |
| if [[ ! -f "$packages_file" && -f "$script_dir/$packages_file" ]]; then | |
| packages_file="$script_dir/$packages_file" | |
| fi | |
| if [[ ! -f "$packages_file" ]]; then | |
| echo "Packages file '$packages_file' not found." | |
| print_usage | |
| exit 1 | |
| fi | |
| # Validate the packages file exists. Relative paths are resolved against the | |
| # current directory first, then against the directory of this script. | |
| if [[ "$first_argument" != "list" ]]; then | |
| if [[ ! -f "$packages_file" && -f "$script_dir/$packages_file" ]]; then | |
| packages_file="$script_dir/$packages_file" | |
| fi | |
| if [[ ! -f "$packages_file" ]]; then | |
| echo "Packages file '$packages_file' not found." | |
| print_usage | |
| exit 1 | |
| fi | |
| fi |
| # CIPD tags are immutable. Verify the tag is unused for every package before | ||
| # downloading anything, so a collision does not leave a partial upload. | ||
| for platform in "${platforms[@]}"; do | ||
| for cipd_name in $(cipd_names_for_platform "$platform"); do | ||
| if describe_output=$(cipd describe "flutter/android/sdk/all/$cipd_name" -version "version:$first_argument" 2>&1); then | ||
| echo "Tag version:$first_argument already exists for flutter/android/sdk/all/$cipd_name." | ||
| echo "CIPD tags are immutable. Please choose a new version tag." | ||
| exit 1 | ||
| elif [[ $describe_output != *"no such tag"* ]]; then | ||
| echo "Failed to check tag version:$first_argument for flutter/android/sdk/all/$cipd_name:" | ||
| echo "$describe_output" | ||
| exit 1 | ||
| fi | ||
| done | ||
| done | ||
| echo "Tag version:$first_argument is unused." |
There was a problem hiding this comment.
When running with --dry-run, the script does not upload any packages to CIPD. However, the cipd describe check is still executed, which requires network access and valid CIPD credentials. If a developer is offline or lacks write/read permissions for these CIPD paths, the dry run will fail. We should skip this tag check when --dry-run is enabled.
| # CIPD tags are immutable. Verify the tag is unused for every package before | |
| # downloading anything, so a collision does not leave a partial upload. | |
| for platform in "${platforms[@]}"; do | |
| for cipd_name in $(cipd_names_for_platform "$platform"); do | |
| if describe_output=$(cipd describe "flutter/android/sdk/all/$cipd_name" -version "version:$first_argument" 2>&1); then | |
| echo "Tag version:$first_argument already exists for flutter/android/sdk/all/$cipd_name." | |
| echo "CIPD tags are immutable. Please choose a new version tag." | |
| exit 1 | |
| elif [[ $describe_output != *"no such tag"* ]]; then | |
| echo "Failed to check tag version:$first_argument for flutter/android/sdk/all/$cipd_name:" | |
| echo "$describe_output" | |
| exit 1 | |
| fi | |
| done | |
| done | |
| echo "Tag version:$first_argument is unused." | |
| # CIPD tags are immutable. Verify the tag is unused for every package before | |
| # downloading anything, so a collision does not leave a partial upload. | |
| if ! $dry_run; then | |
| for platform in "${platforms[@]}"; do | |
| for cipd_name in $(cipd_names_for_platform "$platform"); do | |
| if describe_output=$(cipd describe "flutter/android/sdk/all/$cipd_name" -version "version:$first_argument" 2>&1); then | |
| echo "Tag version:$first_argument already exists for flutter/android/sdk/all/$cipd_name." | |
| echo "CIPD tags are immutable. Please choose a new version tag." | |
| exit 1 | |
| elif [[ $describe_output != *"no such tag"* ]]; then | |
| echo "Failed to check tag version:$first_argument for flutter/android/sdk/all/$cipd_name:" | |
| echo "$describe_output" | |
| exit 1 | |
| fi | |
| done | |
| done | |
| echo "Tag version:$first_argument is unused." | |
| fi |
1 participant
Preview SDKs do not need nor have as many pinned dependencies as the stable android sdk, so I added a path to upload preview SDKs to the existing upload android sdk tooling.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.