This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Protect sdk upload script from missing ndk, add documentation for checking write access, improve comments to add context #47989
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ print_usage () { | |
| echo "" | ||
| echo "This script downloads the packages specified in packages.txt and uploads" | ||
| echo "them to CIPD for linux, mac, and windows." | ||
| echo "To confirm you have write permissions run 'cipd acl-check flutter/android/sdk/all/ -writer'." | ||
| echo "" | ||
| echo "Manage the packages to download in 'packages.txt'. You can use" | ||
| echo "'sdkmanager --list --include_obsolete' in cmdline-tools to list all available packages." | ||
|
|
@@ -26,13 +27,13 @@ print_usage () { | |
| echo "and should only be run on linux or macos hosts." | ||
| } | ||
|
|
||
| # Validate version or argument is provided. | ||
|
Contributor
There was a problem hiding this comment. The following code would read better if you copied $1 into a named variable: VERSION_TAG=$1
Contributor
Author
There was a problem hiding this comment. Named first argument since sometimes that is a version and sometimes it is an argument (which is confusing) but at least the variable hints at the truth. |
||
| if [[ $1 == "" ]]; then | ||
| print_usage | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Validate version contains only lower case letters and numbers. | ||
| if ! [[ $1 =~ ^[[:lower:][:digit:]]+$ ]]; then | ||
| echo "Version tag can only consist of lower case letters and digits."; | ||
| print_usage | ||
|
|
@@ -54,6 +55,8 @@ if [[ ! -d "$sdk_path" ]]; then | |
| print_usage | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Validate caller has cipd. | ||
|
Contributor
There was a problem hiding this comment. What about changing the comment to: Validate environment has cipd installed
Contributor
Author
There was a problem hiding this comment. Done. I like that more. |
||
| if [[ ! -d "$sdk_path/cmdline-tools" ]]; then | ||
| echo "SDK directory does not contain $sdk_path/cmdline-tools." | ||
| print_usage | ||
|
|
@@ -120,12 +123,18 @@ for platform in "${platforms[@]}"; do | |
| # in `ndk/`. | ||
| # This simplifies the build scripts, and enables version difference between | ||
| # the Dart and Flutter build while reusing the same build rules. | ||
| # See https://github.com/flutter/flutter/issues/136666#issuecomment-1805467578 | ||
| mv $upload_dir/sdk/ndk $upload_dir/ndk-bundle | ||
| ndk_sub_paths=`find $upload_dir/ndk-bundle -maxdepth 1 -type d` | ||
| ndk_sub_paths_arr=($ndk_sub_paths) | ||
| mv ${ndk_sub_paths_arr[1]} $upload_dir/ndk | ||
| rm -rf $upload_dir/ndk-bundle | ||
|
|
||
| if [[ ! -d "$upload_dir/ndk" ]]; then | ||
| echo "Failure to bundle ndk for platform" | ||
| exit 1 | ||
|
||
| fi | ||
|
|
||
| # Accept all licenses to ensure they are generated and uploaded. | ||
| yes "y" | $sdkmanager_path --licenses --sdk_root=$sdk_root | ||
| cp -r "$sdk_root/licenses" "$upload_dir/sdk" | ||
|
|
@@ -141,11 +150,16 @@ for platform in "${platforms[@]}"; do | |
| if [[ $platform == "macosx" ]]; then | ||
| cipd_name="mac-$arch" | ||
| fi | ||
|
|
||
| echo "Uploading $upload_dir as $cipd_name to CIPD" | ||
| cipd create -in $upload_dir -name "flutter/android/sdk/all/$cipd_name" -install-mode copy -tag version:$version_tag | ||
| done | ||
|
|
||
| rm -rf $sdk_root | ||
| rm -rf $upload_dir | ||
|
|
||
| # This variable changes the behvaior of sdkmanager. | ||
| # Unset to clean up after script. | ||
| unset REPO_OS_OVERRIDE | ||
| done | ||
| rm -rf $temp_dir | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be done as part of this script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can but honestly given we dont have a dry run. One of the only(and easy) ways to verify what is about to be uploaded is to comment out the
rm -rf $upload_dirand the upload command and see what is about to be uploaded. Adding that check would add another piece of code to comment out. I added this documentation so that when I come back to do flutter/flutter#138021 I know what is required to warn on missing permissions. But in a dry run we would continue anyway since the user is not trying to upload.