Protect sdk upload script from missing ndk, add documentation for checking write access, improve comments to add context #47989

reidbaker · 2023-11-13T21:26:35Z

Script used to upload 34v7 with ndk 26.1.10909125
Added documentation for how to check for write access before running script.
Added documentation for why the ndk is in a non standard location.
Protected against silent ndk failure caused by a failure to download ndk on m1 macs #47609 (comment)
Clean up os override so that script does not dirty the environment variables in a shell where it is run.

See flutter/flutter#117973 for more detail on why a newer ndk is required.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

…cking write access, improve comments to add context

flutter-dashboard · 2023-11-13T21:26:38Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

flutter-dashboard · 2023-11-13T21:26:39Z

This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled.

…failure

bartekpacia · 2023-11-13T21:41:11Z

tools/android_sdk/create_cipd_packages.sh

+  echo "Failure to bundle ndk for platform"
+  exit 1


shouldn't these lines be indented?

btw I wonder if running shellcheck would make sense in Flutter's repos.

Done, if you wanted to add shellcheck I am not opposed. I will say that we (the broader fluter team) have agreed to move from shell scripts like this to dart scripts. Thanks for the catch, this is fixed.

…failure

stuartmorgan · 2023-11-14T15:30:14Z

test-exempt: The current structure of the script (a bash script that does both upload and download) makes it very difficult to test without a complete rewrite, and the team consensus is that the value of such a rewrite is currently too low for the benefits given that this is for an infrequent manual process that already has failsafes later in the process.

johnmccutchan · 2023-11-14T15:36:43Z

tools/android_sdk/create_cipd_packages.sh

@@ -26,13 +27,13 @@ print_usage () {
  echo "and should only be run on linux or macos hosts."
 }

-# Validate version is provided
+# Validate version or argument is provided.


The following code would read better if you copied $1 into a named variable:

VERSION_TAG=$1

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.

johnmccutchan · 2023-11-14T15:38:07Z

tools/android_sdk/create_cipd_packages.sh

@@ -54,6 +55,8 @@ if [[ ! -d "$sdk_path" ]]; then
  print_usage
  exit 1
 fi
+
+# Validate caller has cipd.


What about changing the comment to:

Validate environment has cipd installed

Done. I like that more.

johnmccutchan · 2023-11-14T15:38:56Z

tools/android_sdk/create_cipd_packages.sh

@@ -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'."


Can this be done as part of this script?

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.

…failure

…on for checking write access, improve comments to add context (flutter/engine#47989)

…138751) flutter/engine@2158531...337ab58 2023-11-20 reidbaker@google.com Protect sdk upload script from missing ndk, add documentation for checking write access, improve comments to add context (flutter/engine#47989) 2023-11-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 09bd5d71062d to ee7aaca9adcc (1 revision) (flutter/engine#48237) 2023-11-20 skia-flutter-autoroll@skia.org Roll Skia from 151d297efcf5 to 69213ba6f68a (2 revisions) (flutter/engine#48236) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jonahwilliams@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

Protect sdk upload script from missing ndk, add documentation for che…

c4119ff

…cking write access, improve comments to add context

Remove commented out write check

f3102f1

reidbaker requested review from gmackall and a team November 13, 2023 21:33

Merge branch 'main' into i117973-protect-android-sdk-upload-from-ndk-…

d380914

…failure

bartekpacia reviewed Nov 13, 2023

View reviewed changes

Merge branch 'main' into i117973-protect-android-sdk-upload-from-ndk-…

693ac82

…failure

johnmccutchan reviewed Nov 14, 2023

View reviewed changes

reidbaker added 3 commits November 14, 2023 10:52

Spacing

6ca01ff

Code review feedback

8ddae88

Merge branch 'main' into i117973-protect-android-sdk-upload-from-ndk-…

e0d00fd

…failure

johnmccutchan approved these changes Nov 15, 2023

View reviewed changes

reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 20, 2023

auto-submit bot merged commit 337ab58 into flutter:main Nov 20, 2023
28 checks passed

engine-flutter-autoroll mentioned this pull request Nov 20, 2023

Roll Flutter Engine from 215853171cdc to 337ab58e81f7 (3 revisions) flutter/flutter#138751

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 20, 2023

337ab58e8 Protect sdk upload script from missing ndk, add documentati…

93b367d

…on for checking write access, improve comments to add context (flutter/engine#47989)

reidbaker deleted the i117973-protect-android-sdk-upload-from-ndk-failure branch November 21, 2023 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect sdk upload script from missing ndk, add documentation for checking write access, improve comments to add context #47989

Protect sdk upload script from missing ndk, add documentation for checking write access, improve comments to add context #47989

reidbaker commented Nov 13, 2023 •

edited

Loading

flutter-dashboard bot commented Nov 13, 2023

flutter-dashboard bot commented Nov 13, 2023

bartekpacia Nov 13, 2023

reidbaker Nov 13, 2023

stuartmorgan commented Nov 14, 2023

johnmccutchan Nov 14, 2023

reidbaker Nov 14, 2023

johnmccutchan Nov 14, 2023

reidbaker Nov 14, 2023

johnmccutchan Nov 14, 2023

reidbaker Nov 14, 2023

Protect sdk upload script from missing ndk, add documentation for checking write access, improve comments to add context #47989

Protect sdk upload script from missing ndk, add documentation for checking write access, improve comments to add context #47989

Conversation

reidbaker commented Nov 13, 2023 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Nov 13, 2023

flutter-dashboard bot commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reidbaker commented Nov 13, 2023 •

edited

Loading