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

fixing cirrus flag issue - #20068

Merged
christopherfujino merged 5 commits into
flutter-team-archive:flutter-1.20-candidate.7from
nturgut:flutter-1.20-candidate.7
Jul 28, 2020
Merged

christopherfujino merged 5 commits into
flutter-team-archive:flutter-1.20-candidate.7from
nturgut:flutter-1.20-candidate.7

Conversation

@nturgut

@nturgut nturgut commented Jul 27, 2020 •

Copy link
Copy Markdown
Contributor

Using correct CIRRUS CI flag for getting the branch name.

This fix should have done long ago: https://github.com/flutter/engine/pull/18655/files somehow got forgotten sorry for the inconvenience.

@flutter-dashboard

Copy link
Copy Markdown

This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter.

Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed.

@auto-assign
auto-assign Bot requested a review from iskakaushik July 27, 2020 22:18
@nturgut

nturgut commented Jul 27, 2020

Copy link
Copy Markdown
Contributor Author

Since master is risky, I sent the fix directly to the release branch @pcsosinski @christopherfujino

Comment thread tools/clone_flutter.sh Outdated
# During the commit tests the base branch value is empty, instead
# `CIRRUS_BRANCH` has the correct branch name.
ENGINE_BRANCH_NAME="$CIRRUS_BASE_BRANCH"
if [[ -z "$CIRRUS_BASE_BRANCH" ]] then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is this the only way to determine we're running post submit?

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 only saw two flags in Cirrus. @christopherfujino do you know any other way of determining it?

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 is the only way I know of.

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.

not sure his github handle, so i pinged Fedor in the discord chat

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.

Suggested change
if [[ -z "$CIRRUS_BASE_BRANCH" ]] then
if [[ -z "$CIRRUS_BASE_BRANCH" ]]; then

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.

done. thanks!

@christopherfujino christopherfujino left a comment

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.

LGTM

@nturgut

nturgut commented Jul 27, 2020

Copy link
Copy Markdown
Contributor Author

LGTM

Not sure if merging on release or master is better so I also have this one: #20071

@pcsosinski

Copy link
Copy Markdown

@christopherfujino any thoughts on these two pre submit failures?

@christopherfujino

Copy link
Copy Markdown
Contributor

@christopherfujino any thoughts on these two pre submit failures?

the format_and_dart_test failure seems legit:

+ dart web_sdk/test/api_conform_test.dart
lib/web_ui/lib/ui.dart(2707..2707): This requires the 'non-nullable' language feature to be enabled.
lib/web_ui/lib/ui.dart(5280..5280): This requires the 'non-nullable' language feature to be enabled.
lib/web_ui/lib/ui.dart(1503..1503): This requires the 'non-nullable' language feature to be enabled.
lib/web_ui/lib/ui.dart(5125..5125): This requires the 'non-nullable' language feature to be enabled.
lib/web_ui/lib/ui.dart(3949..3949): This requires the 'non-nullable' language feature to be enabled.
lib/web_ui/lib/ui.dart(1469..1469): This requires the 'non-nullable' language feature to be enabled.
Failure!

and linux web engine also looks legit:

INFO: Driver started
+ [[ '' = false ]]
+ [[ -z '' ]]
+ echo 'Cloning Flutter repo to local machine.'
+ [[ -z /b/s/w/ir/cache/builder ]]
+ cd /b/s/w/ir/cache/builder/src/flutter
+ ENGINE_BRANCH_NAME=
/b/s/w/ir/cache/builder/src/flutter/tools/clone_flutter.sh: line 26: syntax error near unexpected token `then'
Cloning Flutter repo to local machine.
ERROR: Failed to clone flutter repo. Exited with exit code 2

@christopherfujino

Copy link
Copy Markdown
Contributor

I made a suggestion on the PR to fix the Linux web engine failure.

@pcsosinski

Copy link
Copy Markdown

@christopherfujino any thoughts on these two pre submit failures?

the format_and_dart_test failure seems legit:

+ dart web_sdk/test/api_conform_test.dart

I don't think it reflects the state on the branch (https://github.com/flutter/engine/commits/flutter-1.20-candidate.7) (or I am confused):

7/21 test passed when updating the Dart hash to .10 beta (48d9890)

7/27 originally passed all tests (green check) for c88d6b2 and then started failing after the 2.10 tag madness

any chance it's pulling in a dependency from master?

@nturgut

nturgut commented Jul 28, 2020

Copy link
Copy Markdown
Contributor Author

@christopherfujino any thoughts on these two pre submit failures?

the format_and_dart_test failure seems legit:

+ dart web_sdk/test/api_conform_test.dart

I don't think it reflects the state on the branch (https://github.com/flutter/engine/commits/flutter-1.20-candidate.7) (or I am confused):

7/21 test passed when updating the Dart hash to .10 beta (48d9890)

7/27 originally passed all tests (green check) for c88d6b2 and then started failing after the 2.10 tag madness

any chance it's pulling in a dependency from master?

You are right, the format script are apparently added 2 years ago, way before we created the clone_flutter idea.

@tvolkert We wrote clone_flutter script since we were planning to add web integration tests back in March. Do we know why other tests (hots, format) always used master? Shall we try to change it since the master gives an error?

@nturgut

nturgut commented Jul 28, 2020

Copy link
Copy Markdown
Contributor Author

@pcsosinski the Linux Web Test legitimately fails. _platform_web.dart is changed at some point last week, without changing the integration tests. I sent a fix for it on Friday. We need to cherrypick that fix: #20008

@pcsosinski

Copy link
Copy Markdown

@pcsosinski the Linux Web Test legitimately fails. _platform_web.dart is changed at some point last week, without changing the integration tests. I sent a fix for it on Friday. We need to cherrypick that fix: #20008

thanks - but that tells me that the test is the master version. I thought we were branching them?

@tvolkert

Copy link
Copy Markdown
Contributor

@tvolkert We wrote clone_flutter script since we were planning to add web integration tests back in March. Do we know why other tests (hots, format) always used master? Shall we try to change it since the master gives an error?

They used master because they predate our release branches. They should be updated to pull the correct branch. Thanks!

@nturgut

nturgut commented Jul 28, 2020

Copy link
Copy Markdown
Contributor Author

@pcsosinski the Linux Web Test legitimately fails. _platform_web.dart is changed at some point last week, without changing the integration tests. I sent a fix for it on Friday. We need to cherrypick that fix: #20008

thanks - but that tells me that the test is the master version. I thought we were branching them?

Yes, unfortunately I never addressed LUCI.

There is a tracing issue for pre-submit: flutter/flutter#53371

Is it possible to ignore these tests?

@nturgut

nturgut commented Jul 28, 2020

Copy link
Copy Markdown
Contributor Author

@christopherfujino can you please have another look, I added the candidate release names manually as we discussed. fetch_framework one was already edited before. I edited clone_flutter and format scripts.

@nturgut

nturgut commented Jul 28, 2020

Copy link
Copy Markdown
Contributor Author

@jonahwilliams dart web_sdk/test/api_conform_test.dart running under build.sh is failing. Do you know if it is safe to ignore that test and remove it for now?

@pcsosinski I added the candidate branch name to the other places manually.

@christopherfujino
christopherfujino merged commit ac95267 into flutter-team-archive:flutter-1.20-candidate.7 Jul 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

5 participants