Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] Environment variable to disable felt snapshot #13187

Merged
mdebbar merged 2 commits into from
Oct 17, 2019
Merged

[web] Environment variable to disable felt snapshot #13187

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
172ae7d
[web] Environment variable to disable felt snapshot
mdebbar Oct 16, 2019
84cef19
Address Yegor's comments
mdebbar Oct 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions lib/web_ui/dev/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,18 @@ repo. Screenshot tests are compared with the golden files at that revision.
When making engine changes that affect screenshots, first submit a PR to
flutter/goldens updating the screenshots. Then update this file pointing to
the new revision.

## Developing the `felt` tool
If you are making changes in the `felt` tool itself, you need to be aware of Dart snapshots. We create a Dart snapshot of the `felt` tool to make the startup faster.

To make sure you are running the `felt` tool with your changes included, you would need to stop using the snapshot. This can be achived through the environment variable `FELT_USE_SNAPSHOT`:

```
FELT_USE_SNAPSHOT=false felt <command>
```
or
```
FELT_USE_SNAPSHOT=0 felt <command>
```

_**Note**: if `FELT_USE_SNAPSHOT` is omitted or has any value other than "false" or "0", the snapshot mode will be enabled._
5 changes: 4 additions & 1 deletion lib/web_ui/dev/common.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ class _LinuxBinding implements PlatformBinding {

class _MacBinding implements PlatformBinding {
@override
int getChromeBuild(YamlMap browserLock) => browserLock['Mac'];
int getChromeBuild(YamlMap browserLock) {
final YamlMap chromeMap = browserLock['chrome'];
return chromeMap['Mac'];
}

@override
String getChromeDownloadUrl(String version) =>
Expand Down
42 changes: 29 additions & 13 deletions lib/web_ui/dev/felt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,24 +44,40 @@ then
ninja -C $HOST_DEBUG_UNOPT_DIR
fi

# Invalidate cache if:
# * SNAPSHOT_PATH is not a file, or
# * STAMP_PATH is not a file with nonzero size, or
# * Contents of STAMP_PATH is not our local git HEAD revision, or
# * pubspec.yaml last modified after pubspec.lock
#
# To force invalidate the cache run `felt clean` or `rm .dart_tool/felt.shapshot.stamp`
if [[ ! -f $SNAPSHOT_PATH || ! -s "$STAMP_PATH" || "$(cat "$STAMP_PATH")" != "$REVISION" || "$WEB_UI_DIR/pubspec.yaml" -nt "$WEB_UI_DIR/pubspec.lock" ]]; then
echo "Snapshotting the felt tool for faster subsequent runs."
install_deps() {
echo "Running \`pub get\` in 'engine/src/flutter/lib/web_ui'"
(cd "$WEB_UI_DIR"; $DART_SDK_DIR/bin/pub get)

echo "Running \`pub get\` in 'engine/src/flutter/web_sdk/web_engine_tester'"
(cd "$FLUTTER_DIR/web_sdk/web_engine_tester"; $DART_SDK_DIR/bin/pub get)
mkdir -p $DART_TOOL_DIR
}

"$DART_SDK_DIR/bin/dart" --snapshot="$SNAPSHOT_PATH" --packages="$WEB_UI_DIR/.packages" "$SCRIPT_PATH"
echo "$REVISION" > "$STAMP_PATH"
if [[ "$FELT_USE_SNAPSHOT" == "false" || "$FELT_USE_SNAPSHOT" == "0" ]]; then
echo "[Snapshot mode: off]"
# Running without snapshot means there is high likelyhood of local changes. In
# that case, let's clear the snapshot to avoid any surprises.
rm -f "$SNAPSHOT_PATH"

@nturgut nturgut Oct 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Shall we use "rm -rf"?

"-r" argument deletes recursively, may be we might want to use it?

@mdebbar mdebbar Oct 17, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$SNAPSHOT_PATH and $STAMP_PATH point to files, not directories. The recursive flag -r only has an effect on directories, right?

rm -f "$STAMP_PATH"
install_deps
$DART_SDK_DIR/bin/dart "$DEV_DIR/felt.dart" $@
else
# Create a new snapshot if any of the following is true:
# * SNAPSHOT_PATH is not a file, or
# * STAMP_PATH is not a file with nonzero size, or
# * Contents of STAMP_PATH is not our local git HEAD revision, or
# * pubspec.yaml last modified after pubspec.lock
if [[ ! -f $SNAPSHOT_PATH || ! -s "$STAMP_PATH" || "$(cat "$STAMP_PATH")" != "$REVISION" || "$WEB_UI_DIR/pubspec.yaml" -nt "$WEB_UI_DIR/pubspec.lock" ]]; then
echo "[Snapshot mode: on] (creating a new snapshot)"
install_deps
mkdir -p $DART_TOOL_DIR

echo "Creating the snapshot"

@yjbanov yjbanov Oct 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message seems redundant given the message above

"$DART_SDK_DIR/bin/dart" --snapshot="$SNAPSHOT_PATH" --packages="$WEB_UI_DIR/.packages" "$SCRIPT_PATH"
echo "$REVISION" > "$STAMP_PATH"
else
echo "[Snapshot mode: on] (using existing snapshot)"

@yjbanov yjbanov Oct 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default should be silent.

@mdebbar mdebbar Oct 17, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still debating this. On the one hand, like you said, the happy path should be silent. On the other hand, my concern is that someone would make changes in felt and forget to turn off snapshotting. Showing the message all the time makes it easier to realize that the snapshot mode is on.

I'll make it silent for now and see. If we start making a lot of mistakes, I'll "unsilent" it.

@yjbanov yjbanov Oct 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it depends on how often we'll be making changes to felt relative to using it. I'm not terribly attached to one way or the other.

fi

$DART_SDK_DIR/bin/dart --packages="$WEB_UI_DIR/.packages" "$SNAPSHOT_PATH" $@
fi

$DART_SDK_DIR/bin/dart --packages="$WEB_UI_DIR/.packages" "$SNAPSHOT_PATH" $@