-
Notifications
You must be signed in to change notification settings - Fork 6k
Protect sdk upload script from missing ndk, add documentation for checking write access, improve comments to add context #47989
Changes from all commits
c4119ff
f3102f1
d380914
693ac82
6ca01ff
8ddae88
e0d00fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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,34 +27,36 @@ print_usage () { | |
echo "and should only be run on linux or macos hosts." | ||
} | ||
|
||
# Validate version is provided | ||
if [[ $1 == "" ]]; then | ||
first_argument=$1 | ||
# Validate version or argument is provided. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The following code would read better if you copied $1 into a named variable: VERSION_TAG=$1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 [[ $first_argument == "" ]]; then | ||
print_usage | ||
exit 1 | ||
fi | ||
|
||
#Validate version contains only lower case letters and numbers | ||
if ! [[ $1 =~ ^[[:lower:][:digit:]]+$ ]]; then | ||
# Validate version contains only lower case letters and numbers. | ||
if ! [[ $first_argument =~ ^[[:lower:][:digit:]]+$ ]]; then | ||
echo "Version tag can only consist of lower case letters and digits."; | ||
print_usage | ||
exit 1 | ||
fi | ||
|
||
# Validate path contains depot_tools | ||
# Validate environment has cipd installed. | ||
if [[ `which cipd` == "" ]]; then | ||
echo "'cipd' command not found. depot_tools should be on the path." | ||
exit 1 | ||
fi | ||
|
||
sdk_path=${2:-$ANDROID_SDK_ROOT} | ||
version_tag=$1 | ||
|
||
# Validate directory contains all SDK packages | ||
if [[ ! -d "$sdk_path" ]]; then | ||
echo "Android SDK at '$sdk_path' not found." | ||
print_usage | ||
exit 1 | ||
fi | ||
|
||
# Validate caller has cipd. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about changing the comment to: Validate environment has cipd installed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I like that more. |
||
if [[ ! -d "$sdk_path/cmdline-tools" ]]; then | ||
echo "SDK directory does not contain $sdk_path/cmdline-tools." | ||
print_usage | ||
|
@@ -81,7 +84,7 @@ while [ ! -f "$sdkmanager_path" ]; do | |
done | ||
|
||
# list available packages | ||
if [ $version_tag == "list" ]; then | ||
if [ $first_argument == "list" ]; then | ||
$sdkmanager_path --list --include_obsolete | ||
exit 0 | ||
fi | ||
|
@@ -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 $cipd_name to CIPD" | ||
cipd create -in $upload_dir -name "flutter/android/sdk/all/$cipd_name" -install-mode copy -tag version:$version_tag | ||
|
||
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:$first_argument | ||
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 |
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_dir
and 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.