script for fetching correct flutter version #16818

nturgut · 2020-02-26T16:47:25Z

This is a script for fetching the correct version of the flutter framework into engine.

Using "the youngest commit of the framework which is older than engine commit". This will prevent issues which will be caused by cyclic dependency.

…vious step

…way back

….yml to debug better

… directory

Hixie · 2020-02-26T23:05:07Z

The approach seems reasonable to me. Does it work for branches as well?

One thing we might want to do is make the script fail if you're not in CI, because that script looks rather destructive and you wouldn't want people on the team running it accidentally. Maybe set an environment variable in .cirrus.yml and have the script exit with a message if the env var is not set correctly?

cc @tvolkert @pcsosinski since this will affect how tests run on branches.

nturgut · 2020-02-27T00:12:10Z

The approach seems reasonable to me. Does it work for branches as well?

One thing we might want to do is make the script fail if you're not in CI, because that script looks rather destructive and you wouldn't want people on the team running it accidentally. Maybe set an environment variable in .cirrus.yml and have the script exit with a message if the env var is not set correctly?

cc @tvolkert @pcsosinski since this will affect how tests run on branches.

Yes, I agree with you. I fail the code without 'env' and will warn the team member who might try to run it locally. As long as one is careful I think it is ok run the script locally, for example, to test what is failing on CI.

(Unit tests are failing since the template was common between tests, I'll address it in the next commit)

tvolkert · 2020-02-27T17:19:02Z

tools/clone_flutter.sh

+cd flutter
+# Get the time of the youngest commit older than engine commit.
+# Before makes the comparison considering the timezone as well.
+COMMIT_NO=`git log --before="$LATEST_COMMIT_TIME_ENGINE" -n 1 | grep commit | cut -d ' ' -f2`


What happens if the latest engine commit is after the latest framework commit? Will this say there's no commit to reset to? If it leaves the framework at ToT, then multiple runs of this script at the same engine commit could still yield different framework versions...

This is deterministically getting the youngest framework commit older than the engine commit. For example engine commit is 10pm PST. The framework tree was red for half a day. The framework commit will be 10am PST.

Yes you are right. Different commits running at the same time on cirrus, since their head is different, will be fetching a different version of the framework repo.

Ah cool - at first I read it as "oldest commit younger than the engine commit".

So yes, next question is about branches. How does this behave if you're running on a release branch in the engine? We may want to have this detect if you're not on master, and then fetch the branch by the same name in the framework (and fail if there's no branch by the same name in the framework).

Should we worry about https://stackoverflow.com/questions/12934044/git-parent-commit-is-younger-than-descendant? Perhaps topological age should take precedence over actual date?

@tvolkert How do we run tests on CI for branches? Also, how do we keep LUCI recipes stable? A change in LUCI recipe can break our tests in older commits too (in fact it did just last week).

On flutter/engine there isn't any branches for 'dev', 'beta', 'stable'. Is the suggestion for managing the hotfixes?

@yjbanov On stackoverflow article:

The commit date can be changed that is true. On the other hand we are simply trying to prevent both trees being closed on an error on flutter/flutter. We can add this to the Guidelines and simply ask the engine contributors not to change the date.

git log --before and --after comments are always using commit date, so all the other parts on the stackoverflow discussion regarding the rebases are irrelevant to line 21 of the script.

@yjbanov infra team is working on the ability to pin the recipe version to the engine.

@nturgut these won't be dev, beta, or stable - they'll be release branches (one per dev release, created on both the framework and the engine).

@Hixie @tvolkert I added the checks for the branch. @yjbanov I added a comment for your concern.

Even though this script works locally, I am not sure how it will run for the release process with cirrus CI. as far I as understand cirrus only runs on detached head. 'master' branches is used for post submit benchmarks. Once the cloning is done, there isn't any other configuration for a specific branch.

In my opinion we should either use LUCI for the release process or change the clone script of cirrus https://cirrus-ci.org/guide/tips-and-tricks/#custom-clone-command

Looking at the github history I couldn't find any instances when a child commit date is older than the parent's, so it looks like Github's "squash and merge" button does the right thing for us. This can only happen if someone pushes using the command line, and they would really have to go out of their way to change the dates. I think we're good.

Recipe pinning SGTM. Or better, move them into the respective repos.

tvolkert · 2020-02-27T17:21:45Z

Is the idea here solely to ensure that we pin the version of the framework we sync to to make sure the world isn't shifting under our feet when we run engine tests?

nturgut · 2020-02-27T18:38:58Z

Is the idea here solely to ensure that we pin the version of the framework we sync to to make sure the world isn't shifting under our feet when we run engine tests?

Yes. we don't want to block all the engine merges if there is a breakage in flutter/flutter which occurred later than the engine commit time.

yjbanov · 2020-02-27T21:48:26Z

tools/clone_flutter.sh

@@ -0,0 +1,24 @@
+#!/bin/bash
+set -e


I'd also add set -x so all the commands we run here are logged

thanks for the suggestion!

yjbanov · 2020-02-27T22:03:17Z

tools/clone_flutter.sh

+cd flutter
+# Get the time of the youngest commit older than engine commit.
+# Before makes the comparison considering the timezone as well.
+COMMIT_NO=`git log --before="$LATEST_COMMIT_TIME_ENGINE" -n 1 | grep commit | cut -d ' ' -f2`


Should we worry about https://stackoverflow.com/questions/12934044/git-parent-commit-is-younger-than-descendant? Perhaps topological age should take precedence over actual date?

@tvolkert How do we run tests on CI for branches? Also, how do we keep LUCI recipes stable? A change in LUCI recipe can break our tests in older commits too (in fact it did just last week).

tools/clone_flutter.sh

tvolkert · 2020-02-28T17:12:25Z

tools/clone_flutter.sh

+  echo "Switching branches on Framework to from $FRAMEWORK_BRANCH_NAME to $ENGINE_BRANCH_NAME"
+  # Switch to the same version branch with the engine.
+  # If same version branch does not exits, fail.
+  SWITCH_RESULT=`git checkout $ENGINE_BRANCH_NAME` || true


Wont || true make this always true?

Yes. If we don't put it, when there is an error (such as branch not found) the script automatically ends without printing out the logs I added.

When we use this if the branch exists the SWITCH_RESULT is a non empty string if it fails it is an empty string

tvolkert · 2020-02-28T17:12:59Z

LGTM modulo comments.

yjbanov

LGTM if LGT @tvolkert

nturgut · 2020-02-28T20:30:45Z

LGTM if LGT @tvolkert

thanks!

nturgut · 2020-02-28T20:51:41Z

LGTM if LGT @tvolkert

thanks!

Merging the PR, got approval from Todd.

…6818)

* 9beac71 Add support for software text editing controls (flutter/engine#15560) * ab9f83f Roll fuchsia/sdk/core/linux-amd64 from RYDur... to bgFop... (flutter/engine#16855) * 62d0c08 Roll src/third_party/dart dda5bcee00d3..4dad6d77ba50 (6 commits) (flutter/engine#16856) * 76d12d2 Roll src/third_party/skia 03d9e8af0d25..262796edeba6 (11 commits) (flutter/engine#16857) * 01a52b9 Try rasterizing images and layers only once, even when their rasterization fails. Further enforce the same access threshold on layers as on Pictures. Previously layers would always be cached. The latter is a semantic change. (flutter/engine#16545) * 3b0d1a8 script for fetching correct flutter version (flutter/engine#16818) * 9746ddb Roll src/third_party/dart 4dad6d77ba50..6708f6d4c7df (15 commits) (flutter/engine#16860) * 90736bb Roll src/third_party/skia 262796edeba6..54cb21430ccb (23 commits) (flutter/engine#16861) * 142882e Roll src/third_party/skia 54cb21430ccb..e1ae9c4bcf2e (4 commits) (flutter/engine#16863) * 7192356 Manual roll of Dart 09bbd3cca5...6708f6d4c7 (flutter/engine#16864) * fae5ff6 Roll src/third_party/skia e1ae9c4bcf2e..5d1c3e2ead61 (2 commits) (flutter/engine#16865) * c6525a4 Roll src/third_party/skia 5d1c3e2ead61..59b160f99106 (2 commits) (flutter/engine#16866) * 755e2b5 Roll src/third_party/skia 59b160f99106..71a20b2685c6 (1 commits) (flutter/engine#16867) * 93d0eff Roll src/third_party/skia 71a20b2685c6..ecbb0fb2d5bc (1 commits) (flutter/engine#16869) * bfee5ae Roll fuchsia/sdk/core/linux-amd64 from bgFop... to F_Ihm... (flutter/engine#16871) * ca29b47 Roll fuchsia/sdk/core/mac-amd64 from K26F5... to 79I0C... (flutter/engine#16872) * 708ca2c Roll src/third_party/skia ecbb0fb2d5bc..666707336e07 (1 commits) (flutter/engine#16873) * 78ea291 Roll fuchsia/sdk/core/mac-amd64 from 79I0C... to NoQzJ... (flutter/engine#16874) * 7b43bb8 Roll fuchsia/sdk/core/linux-amd64 from F_Ihm... to 22R78... (flutter/engine#16875) * 75c9c62 Roll src/third_party/skia 666707336e07..231f1bf56556 (1 commits) (flutter/engine#16876) * 7a86b83 Roll src/third_party/dart 09bbd3cca5d4..860dca93ea42 (1 commits) (flutter/engine#16877) * d0c87e2 Roll src/third_party/skia 231f1bf56556..d6205322cdc5 (1 commits) (flutter/engine#16878) * b4c5854 Roll src/third_party/skia d6205322cdc5..6729496a037f (1 commits) (flutter/engine#16879) * 03f0ee5 Roll src/third_party/skia 6729496a037f..367dbff98555 (1 commits) (flutter/engine#16880) * d84b968 Roll fuchsia/sdk/core/mac-amd64 from NoQzJ... to q2DAy... (flutter/engine#16881) * 4490e7c Roll fuchsia/sdk/core/linux-amd64 from 22R78... to 9NHsJ... (flutter/engine#16882) * a39e7ac Roll src/third_party/skia 367dbff98555..986680240f81 (1 commits) (flutter/engine#16884) * 62b1e50 Roll src/third_party/dart 860dca93ea42..fbe9f6115d2f (9 commits) (flutter/engine#16885) * 5e474ee Roll src/third_party/skia 986680240f81..9dd0bd78b2d7 (2 commits) (flutter/engine#16886)

script for fetching correct flutter version

de67def

googlebot added the cla: yes label Feb 26, 2020

nturgut added 13 commits February 26, 2020 09:10

change cirrus yml for the script location

aaf5c00

change location of the script. Add it to tools

ab4396b

cirrus still does not see the script. repeat path change from the pre…

fd54619

…vious step

Looks like script worked correctly. do not change directory.

bc4bc65

change directory back to build root after scriot is run

66595f0

script runs ok. Still not able to find the bin directory. go all the …

2dd118c

…way back

still can't see the bin directory. carry the script content to cirrus…

5c49aa5

….yml to debug better

get the last commit id of the right repository

e60e862

content of the script worked in cirrus. call the script from tools

7a1325f

cannot find the script under tools

6d3afaa

print the current path in the script to see why cirrus cannot see bin…

63c3a8a

… directory

move to flutter path before running update packages

939a9c0

tests run now. remove print outs

daa8884

nturgut requested review from Hixie, tvolkert and yjbanov February 26, 2020 21:40

tvolkert reviewed Feb 27, 2020

View reviewed changes

yjbanov reviewed Feb 27, 2020

View reviewed changes

nturgut added 6 commits February 27, 2020 14:15

error if the ENGINE_PATH is not set. exit the script

5887ede

addressing reviewer comments

4c9fce0

engine branch name on cirrus logs doesn't make sense

e88bcb2

fix typo

4712ab5

change the directory of branch calculation

1fc87e9

remove extra logs

618a176

nturgut requested review from tvolkert and yjbanov February 28, 2020 03:01

tvolkert reviewed Feb 28, 2020

View reviewed changes

tools/clone_flutter.sh Outdated Show resolved Hide resolved

tvolkert reviewed Feb 28, 2020

View reviewed changes

tools/clone_flutter.sh Outdated Show resolved Hide resolved

tvolkert reviewed Feb 28, 2020

View reviewed changes

tools/clone_flutter.sh Outdated Show resolved Hide resolved

tvolkert reviewed Feb 28, 2020

View reviewed changes

nturgut added 2 commits February 28, 2020 10:43

addressing PR comments. Testing CIRRUS_CI env variable

c39a4ae

adding CIRRUS_CI check

156bd65

yjbanov approved these changes Feb 28, 2020

View reviewed changes

nturgut merged commit 3b0d1a8 into flutter:master Feb 28, 2020

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 29, 2020

3b0d1a8 script for fetching correct flutter version (flutter/engine#1…

68d5862

…6818)

engine-flutter-autoroll mentioned this pull request Feb 29, 2020

Roll engine 96061d62281d..142882e1baea (9 commits) flutter/flutter#51689

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 2, 2020

3b0d1a8 script for fetching correct flutter version (flutter/engine#1…

d585015

…6818)

engine-flutter-autoroll mentioned this pull request Mar 2, 2020

Roll engine 96061d62281d..5e474ee860a3 (29 commits) flutter/flutter#51792

Merged

nturgut mentioned this pull request Mar 3, 2020

Fetch youngest flutter/flutter older than flutter/engine for integration tests flutter/flutter#51426

Closed

nturgut mentioned this pull request Apr 7, 2020

use a fixed version of flutter in the tests running on the engine #17582

Closed

script for fetching correct flutter version #16818

script for fetching correct flutter version #16818

Uh oh!

Conversation

nturgut commented Feb 26, 2020

Uh oh!

Hixie commented Feb 26, 2020

Uh oh!

nturgut commented Feb 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Feb 27, 2020

Uh oh!

nturgut commented Feb 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Feb 28, 2020

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

nturgut commented Feb 28, 2020

Uh oh!

nturgut commented Feb 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants