Add skp_generator package #91
Conversation
0f6ea1b to
89327e4
Compare
|
Fixes flutter/flutter#80884 |
skp_generator/README.md
Outdated
| ## Contributing new scenes | ||
|
|
||
| Contributions of scenes are welcome. New scenes should be | ||
| significantly distinct from all existing scenes, and should either |
There was a problem hiding this comment.
This will be hard to tell without Skia's help. Scenes that visually look pretty different may boil down to the same ops, and scenes that look almost exactly the same visually can boil down to very different ops.
Not sure what we can do about that, but will we be able to get feedback upstream from Skia if we're submitting substantially similar SKPs?
There was a problem hiding this comment.
I actually meant specifically different at the Flutter level (e.g. I don't want 200 scenes that all boil down to an app bar with text in the center). The point here is to have representation of typical Flutter scenes, while being agnostic about how we generate those for Skia and agnostic about how Skia handles them -- so they might generate similar SKPs today, but not tomorrow.
There was a problem hiding this comment.
Clarified to:
Contributions of scenes are welcome. New scenes should be
significantly distinct from existing scenes (in terms of visuals and
Flutter widgets used to create them), and should either show a scene
that is expensive to rasterize, or show an interesting or complex
scene from a real-world application.
dnfield
left a comment
There was a problem hiding this comment.
I think we should include a pubspec with this that specifies a minimum version constraint for when we started letting flutter_tester capture SKPs. Otherwise this LGTM with some nits.
|
|
||
| If you have animations, consider placing all the frames into one image | ||
| (side by side) to simulate the cost of rendering the entire animation. | ||
|
|
There was a problem hiding this comment.
SKPs embed image and font data. It might be fine to wait until we have this problem, but if people are submitting large numbers of scenes that have large chunks of image and font data, it may be worth replacing images with the smallest possible representation that preserves the size of the image (actual pixels don't matter for shader compilation).
For fonts we don't have a great option at this point other than to include them, see https://bugs.chromium.org/p/skia/issues/detail?id=10404
There was a problem hiding this comment.
What problem would we be solving here?
There was a problem hiding this comment.
Binary size of the SKPs. This is probably fine to ignore until that becomes a problem, and it might not ever.
There was a problem hiding this comment.
Yeah I haven't quite figured out where the SKPs end up living right now but I'm not aware of size being an issue we need to care about yet.
skp_generator/build.sh
Outdated
| } | ||
|
|
||
| cleanup | ||
| #trap cleanup EXIT # COMMENT OUT THIS LINE TO LEAVE DEVELOPMENT ARTIFACTS (see README) |
There was a problem hiding this comment.
nit: rather than leaving ac ommented out line, should we just have an argument to the script determine this?
Like sh build.sh --no-clean or something?
There was a problem hiding this comment.
i did not mean to leave this commented out oops
There was a problem hiding this comment.
Added support for --no-clean and updated README.
| @@ -0,0 +1,66 @@ | |||
| # Copyright 2014 The Flutter Authors. All rights reserved. | |||
skp_generator/build.sh
Outdated
| # The output will be placed in the skps/ directory. | ||
| # We use --run-forever because the app terminates itself when it's done. | ||
| # We use --non-interactive for obvious reasons. | ||
| $(dirname $(which flutter))/cache/artifacts/engine/$PLATFORM/flutter_tester --run-forever --non-interactive --packages=.dart_tool/package_config.json --flutter-assets-dir=build/flutter_assets/ build/flutter_assets/kernel_blob.bin |
There was a problem hiding this comment.
I think this might fail if you're using a symlink. @gspencergoog ?
There was a problem hiding this comment.
Yeah, it probably would. You'll probably want to use type -P flutter instead of which (since which isn't the same everywhere), and then do something like this to find the actual directory where it exists. I know this seems overly complicated, but we've looked at a lot of options, and this worked most reliably on the most systems for finding the location of the actual file so you could take its dirname safely.
# Needed because if it is set (e.g. incorrectly exported), cd may print the
# path it changed to.
unset CDPATH
# On Mac OS, readlink -f doesn't work, so follow_links traverses the path one
# link at a time, and then cds into the link destination and find out where it
# ends up.
#
# The returned filesystem path must be a format usable by Dart's URI parser,
# since the Dart command line tool treats its argument as a file URI, not a
# filename. For instance, multiple consecutive slashes should be reduced to a
# single slash, since double-slashes indicate a URI "authority", and these are
# supposed to be filenames. There is an edge case where this will return
# multiple slashes: when the input resolves to the root directory. However, if
# that were the case, we wouldn't be running this shell, so we don't do anything
# about it.
#
# The function is enclosed in a subshell to avoid changing the working directory
# of the caller.
function follow_links() (
cd -P "$(dirname -- "$1")"
file="$PWD/$(basename -- "$1")"
while [[ -h "$file" ]]; do
cd -P "$(dirname -- "$file")"
file="$(readlink -- "$file")"
cd -P "$(dirname -- "$file")"
file="$PWD/$(basename -- "$file")"
done
echo "$file"
)
# Absolute path to actual flutter location.
FLUTTER="$(follow_links "$(type -P flutter)")"
BIN_DIR="$(dirname -- "$FLUTTER")"There was a problem hiding this comment.
I'm not sure what problem we're trying to solve here. The use case for this script is Flutter team members who work on Flutter, and CI. In CI the Flutter tool is downloaded by this script itself, no symlinks involved. For Flutter team members, the workflow is to have the flutter shell script on your path, no symlinks involved.
There was a problem hiding this comment.
I've integrated the code above into the script.
There was a problem hiding this comment.
FWIW, I have had setups for flutter in the past that had a link to the bin/flutter script from a cmd directory that was in my PATH. I don't have that setup anymore, but I don't think it's unreasonable.
There was a problem hiding this comment.
Yeah, we have this kind of logic in most of the Flutter shell scripts so that people who symlink their flutter SDK to more rapidly switch between different versions (without downloading all the artifacts for each switch) don't get broken.
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ' | ||
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ' | ||
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ' | ||
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ' | ||
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ', |
There was a problem hiding this comment.
nit:
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ' | |
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ' | |
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ' | |
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ' | |
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ', | |
| 'This is lots of dummy text. This is lots of dummy text. This is lots of dummy text. ' * 5, |
There was a problem hiding this comment.
Well if we're going down that road, let's just make it one copy of the sentence * 15 :-)
| color: const Color(0xFF006666), | ||
| border: Border.all(color: const Color(0xFF0000FF), width: 8.0), | ||
| ), | ||
| child: Text('My name\'s not Kirk... It\'s Skywalker.', |
There was a problem hiding this comment.
nit: consider either ''' or " instead of escaping the '.
There was a problem hiding this comment.
I find it easier to read code if all strings always use the same syntax and escaping than if there's different kinds of strings used everywhere. If it were up to me I'd ban use of ", ''', and probably even r' though maybe allowing it specifically for RegExps...
|
Support was added in flutter/flutter@4214657, which is not yet on a tagged version. |
|
What problem would having a I wanted to avoid having anything that wasn't strictly necessary because that way it would evolve with any changes we make. For example, if for some reason we remove the cupertino font dependency by default, then this would also lose it. If we add some other dependency, it would gain it. If we change the default analysis options, this would be affected. And so on. Since this downloads the latest master on run, it will always be used after the version that has the relevant support. |
|
(Updated commit pushed.) |
|
The pubspec shouldn't affect analysis options right? My main concern is that if someone runs this application with too-early a version of Flutter, it will fail to get the SKP and may not immediately be obvious as to why. If the version constraint is there, it would fail on |
|
This is intended to be used with master (I intend to add this as a shard over on flutter/flutter). I can certainly document that if it doesn't work you should check you're on master. |
|
In that case, is this test opted out for dev/beta/stable pushes? |
|
Test doesn't exist at all yet but that's definitely something to keep in mind when adding it, yeah. |
@dnfield dnfield Update flutter_svg.test (flutter#125) e7e9f69 3 days ago Git stats 114 commits Files Type Name Latest commit message Commit time registry Update flutter_svg.test (flutter#125) 3 days ago scripts Update certificates for windows bots (flutter#101) 10 months ago skp_generator Run dart fix --apply before creating SKPs (flutter#112) 8 months ago .cirrus.yml Add skp_generator package (flutter#91) 10 months ago .gitignore Document the process and get things ready 3 years ago README.md update everything to tip of trees (flutter#111) 8 months ago README.md Flutter Customer Test Registry This repository contains references to tests (in the registry directory) that are run with every commit to Flutter to verify that no breaking changes have been introduced (in the "customer_testing" shards). The tests referenced by this repository are typically maintained by people outside of the Flutter team, as part of the development of their applications. They are intended to give the Flutter team visibility into how their changes affect real-world developers using Flutter. Adding more tests You are welcome to add a new batch of tests. To do so, copy the file registry/template.test to create a new file in the registry/ directory. Fill in the fields and delete all the comments. Then, submit a PR with this new file. Tests must fulfill the following criteria to be added: All the code must be available publicly on GitHub under a license compatible with this effort. Tests must be hermetic. For example, a test should not involve network activity, spawn processes, or access the local file system except to access files that are packaged with the test. Tests must be resilient to being run concurrently with other tests, including concurrent runs of themselves. Tests must be reliable. A test must not claim to pass if it is failing. Running a test multiple times in a row must always have the same result. Tests must have no output when they are passing. Tests must be as fast as possible given the hardware. For example, tests must not use real timers or depend on the wall clock. The time taken by tests must be proportional to their value. A few thousands tests are expected to run within a few minutes. An upper limit of about five minutes will be applied to each contributed test suite (not including the time to download the tests), but it is expected that most suites will complete in seconds. The tests must be compatible with any tools for automatically updating Flutter code (e.g. they cannot rely on custom code generation unless such code generation can hook into the automatic update mechanism). The tests must represent good practices as described in Flutter's documentation. For example, using an object after calling its dispose method violates the contract described by that method. Accessing the fields of a private State subclass from another package by casting it to dynamic is similarly sketchy and would not be supported behaviour. The tests must pass even if there are analysis 'info' level issues in the code. Generally, this means that if the test performs static analysis, it does so ignoring info level items (i.e., flutter analyze --no-fatal-infos). The tests must pass at the time they are contributed. The upstream repository that hosts the tests must be able to receive patches to support the master channel of Flutter. This means that CI on the upstream repository should use the master channel Flutter SDK. Dependencies must be pinned. (Generally, checking in the pubspec.lock file is sufficient for this purpose.) Running the tests locally To run these tests locally, check out this directory in a directory parallel to your flutter repository checkout, then, from this directory, run: pushd ../flutter/dev/customer_testing && pub get && popd ../flutter/bin/cache/dart-sdk/bin/dart ../flutter/dev/customer_testing/run_tests.dart --skip-template --verbose registry/*.test The first command retrieves the Dart packages used by customer_testing and can be omitted for subsequent executions. If a test is broken The point of these tests is to make sure we don't break existing code, while still being able to make improvements to Flutter APIs. If you find that a PR you have created in flutter/flutter causes one these tests to fail, you have the following options: Change your PR so that the test no longer fails. This is the preferred option, so long as the result is one we can be proud of. Is the resulting API something that you would plausibly come up with without the backwards-compatibility constraint? That's good. Is the resulting API something that, as soon as you see it, you think "why?" or "that's weird"? That's bad. Consider the advice in the Style guide: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo Go through the breaking change process, as documented here: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes If you're going to do this, you will need to contact the relevant people responsible for the breaking test(s) (see the relevant .test files), help them fix their code, and update this repository to use the new version of their tests, in addition to the steps described on the wiki. You will also need to land your change in two parts, so that people have time to migrate (a "soft-breaking" change). Remove the test in question. This is by far the least ideal solution. To go down this path, we must first establish that one of the following is true: the people listed as contacts for the test are not responsive (within 72 hours). the test is poorly written (e.g. it contains a race condition or relies on assumptions that violate clearly documented API contracts), and the people listed as contacts are not willing to fix the test or accept fixes for the test. we have gone through the breaking change process cited above, but are unable to update the test accordingly (e.g. the people listed as contacts are not willing to work with us to update their code). SKP Generators The skp_generator directory contains a Flutter program (and associated shell scripts) to generate test SKPs for the Skia team. Contributions in the form of stateless widgets showing scenes from your application are welcome. See the README.md in that directory.
This script generates SKP images from various Flutter scenes for testing in Skia's automated systems.
Fixes flutter/flutter#80884