Clean up scenario app scripts #20641
Conversation
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
I think this is probably fine, but @blasten has been doing work getting this stuff running on CI recently and should probably have a look.
It would be good if we could make sure that whatever this does is the same as what's getting done on CI.
|
If these should just be deleted in favor of running a CI script instead, that's fine too. It's confusing to have multiple ways to test something. |
| popd | ||
| SCRIPT_DIR=$(follow_links "$(dirname -- "${BASH_SOURCE[0]}")") | ||
| SRC_DIR="$(cd "$SCRIPT_DIR/../../.."; pwd -P)" | ||
| export ANDROID_HOME="$SRC_DIR/third_party/android_tools/sdk" |
There was a problem hiding this comment.
Nice. this aligns with what the LUCI recipe does. It's good that it makes it consistent.
| mkdir -p "$OUTDIR" | ||
| mkdir -p "$FLUTTER_ASSETS_DIR" | ||
| mkdir -p "$LIBS_DIR" |
There was a problem hiding this comment.
I missed these quotes before. Thanks for fixing!
|
Some of these scripts are called by the recipe code. @gspencergoog do you have any thoughts on migrating these bash script to Dart? I saw you did something similar recently. Maybe a unified package that can be used by the Flutter tool as well as the engine scenarios? I'm not sure if the gain would justify the work, but just curious if you have any thoughts. Also, cc @jonahwilliams |
gspencergoog
force-pushed
the
scenario_scripts
branch
2 times, most recently
from
93dabe2 to
77514a0
Compare
|
@blasten They could probably be rewritten in Dart pretty easily. As for whether it is worth it, I think it depends on what the ultimate goal is for these: are they just stopgap scripts until a real build system/recipe is used to build them, or are the meant to be "the way" to build/run these tests? |
|
My understanding is that the engine repo intentionally does not use the tool, but that would otherwise be a reasonable way to build the apps. I can't see a situation in which the tool delegates building apps to another package though, that would really increase the difficulty in making/rolling changes through |
|
At one point there were thoughts of rewriting these in GN (and probably python) so that it would use the same build system as the rest of the repo. I think that might be preferable to Dart in this case, but haven't had time to look at it more seriously. |
gspencergoog
force-pushed
the
scenario_scripts
branch
2 times, most recently
from
aadb9a9 to
cd23add
Compare
|
OK, well, for now I'm content with just improving their usability. LGTM anyone? |
gspencergoog
force-pushed
the
scenario_scripts
branch
from
cd23add to
97e45c4
Compare
|
@blasten Not at all. Done. |
5 participants
Description
I just made a pass on the scenario scripts so that they can be more easily run from the scenario directory, set the
ANDROID_HOMEcorrectly, and generally fixed lint errors.Also
compile_android_aot.shdidn't appear to work, and I think I fixed it (it builds now), but that change might not be right (it wasn't pointing togen_snapshotproperly, or toflutter.jar).I realize that these are mainly temporary scripts, but I needed to run them to test a change, and was having a hard time getting them to run.