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

fixing cirrus flag issue #20068

Conversation

nturgut
Copy link
Contributor

@nturgut nturgut commented Jul 27, 2020

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

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
Copy link
Contributor Author

nturgut commented Jul 27, 2020

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

# 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

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
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
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
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
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
Contributor Author

Choose a reason for hiding this comment

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

done. thanks!

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

@nturgut
Copy link
Contributor Author

nturgut commented Jul 27, 2020

LGTM

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

@pcsosinski
Copy link

@christopherfujino any thoughts on these two pre submit failures?

@christopherfujino
Copy link
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
Contributor

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

@pcsosinski
Copy link

@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
Copy link
Contributor Author

nturgut commented Jul 28, 2020

@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
Copy link
Contributor Author

nturgut commented Jul 28, 2020

@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

@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
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
Copy link
Contributor Author

nturgut commented Jul 28, 2020

@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
Copy link
Contributor Author

nturgut commented Jul 28, 2020

@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
Copy link
Contributor Author

nturgut commented Jul 28, 2020

@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: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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants