[Fuchsia] Remove the use of //build/ in fuchsia #51072

zijiehe-google-com · 2024-02-28T22:46:55Z

Motivation of the change:

Both dart and flutter are using fairly outdated gn-sdk without properly maintained. Currently @hjfreyer is working on version'ed IDK / SDK libs which requires changes in gn-sdk to use the right version of the libs in fuchsia/sdk/obj/{arch}-api-{level} rather than the one in the fuchsia/sdk/arch. But current implementation does not support choosing the right version.

Blocking issue:

The new gn-sdk (in flutter/tools/fuchsia/gn-sdk) generates multiple BUILD.gn files rather than a large BUILD.gn the previous version created. So most of the build rules need to switch from the old fidl:{api} build rule to fidl/{api} rule. The same change will happen in the dart/sdk, i.e. http://go/dart-reviews/356924. But since the two repos cannot have one single atomic change, changing either side first will cause flutter to break. E.g. the linkage error caused by duplicated symbols will happen if we change the dart/sdk first, since in flutter, it will still refer to the old build rules in the middle.

Solutions:

Ideally we can create redirect rules in the current build/fuchsia buildroot tree to redirect the old rules into the new one, so we can make the change in the flutter first then dart/sdk. But creating the rules is not trivial and will only be used once.

So an alternative solution is

pause the dart/sdk -> flutter roll
submit dart/sdk change (http://go/dart-reviews/356924)
update this change to manually bring the dart/sdk change, namely the dart_revision in the DEPS file and signatures in the ci/licences.
resume the dart/sdk -> flutter roll.

But it requires this change itself to be reviewed first, and I'd like to know your opinion before moving forward.

See corresponding dart/sdk change at http://go/dart-reviews/356924.

//build/fuchsia/ from buildroot should be removed after this change.

Bug: b/40935282

FYI: @hjfreyer

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2024-03-13T22:12:43Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #51072 at sha 7aa1ac1

flutter-dashboard · 2024-03-13T22:50:10Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

zanderso

I don't fully understand the plan for landing this. There are currently merge conflicts, but the plan will be to revert eagerly if anything goes wrong.

zijiehe-google-com · 2024-03-14T20:05:10Z

Yes, the merge conflict will be addressed once we have a new dart version from the change http://go/dart-reviews/356924.

This change may require a codereview from flutter side before moving forward. See flutter/engine#51072. This reverts commit 283051a. Reason for revert: Redo the change with the similar one in flutter. After this change, the next dart -> flutter roll needs extra cares in flutter/engine#51072. It also contains more details about the reason why it could not be two-way compatible. Original change's description: > Revert "[Fuchsia] Remove the legacy gn build rules for fuchsia" > > This reverts commit 094202b. > > Reason for revert: Break flutter, this change should happen after > flutter being migrated to the same gn-sdk. > > Original change's description: > > [Fuchsia] Remove the legacy gn build rules for fuchsia > > > > The updated test-scripts contains https://crrev.com/c/5341620 which > > allows to generate fidl apis with an env var controlled location > > rather than a hard-coded one. So the existing fuchsia gn build rules > > in dart/sdk become obsolete and can be fully removed in favor of the > > one in //third_party/fuchsia/gn-sdk/. > > > > Meanwhile the gn-sdk has been updated with > > https://crrev.com/c/5325282 to use api-level version'ed idk but not > > the ones in arch/. > > > > Cq-Include-Trybots: luci.dart.try:vm-fuchsia-release-arm64-try,vm-fuchsia-release-x64-try > > Tested: ^^^^^ > > Bug: 40935282 > > Change-Id: I2ce958e6db1ff8221beef7b7ff953c32bb4525ba > > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/355283 > > Reviewed-by: Ryan Macnak <rmacnak@google.com> > > Reviewed-by: Alexander Thomas <athom@google.com> > > Commit-Queue: Zijie He <zijiehe@google.com> > > Bug: 40935282 > Change-Id: I7c455d1d362210523671c97d99ef018ede1743f4 > Cq-Include-Trybots: luci.dart.try:vm-fuchsia-release-arm64-try,vm-fuchsia-release-x64-try > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/356307 > Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> > Auto-Submit: Zijie He <zijiehe@google.com> > Reviewed-by: Derek Xu <derekx@google.com> > Commit-Queue: Zijie He <zijiehe@google.com> Bug: 40935282 Change-Id: Id5a9a98013350359037b19f8506f24158c9a3120 Cq-Include-Trybots: luci.dart.try:vm-fuchsia-release-arm64-try,vm-fuchsia-release-x64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/356924 Commit-Queue: Zijie He <zijiehe@google.com> Reviewed-by: Ryan Macnak <rmacnak@google.com>

zijiehe-google-com · 2024-03-15T02:38:46Z

#51437 should be covered by this change.

…gine#51072)

…145196) flutter/engine@995d890...1e199ce 2024-03-15 68449066+zijiehe-google-com@users.noreply.github.com [Fuchsia] Remove the use of //build/ in fuchsia (flutter/engine#51072) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This is a quick follow up of #51072 to only download fuchsia deps (via download_fuchsia_deps gclient arg) when necessary. Considering the pr/51072 is very huge, keeping these condition changes in a separate change would be cleaner. The wiki has been updated already since setting the download_fuchsia_deps gclient arg explicitly won't break the existing workflow. I also mentioned in the wiki that building and running fuchsia needed a linux box (an explicit assertion in the with_envs.py). Bug: http://b/40935282 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

After flutter/engine#51072, this folder is obsolete and can be removed. The change is locally tested. Bug: http://b/40935282 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read the [Flutter Style Guide] _recently_, and have followed its advice. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --------- Co-authored-by: Jason Simmons <jason-simmons@users.noreply.github.com>

#51500) Reverts: #51439 Initiated by: zijiehe-google-com Reason for reverting: broke dart->flutter roller due to the expected licenses not being included without `download_fuchsia_deps` in `.gclient`. Original PR Author: zijiehe-google-com Reviewed By: {zanderso} This change reverts the following previous change: This is a quick follow up of #51072 to only download fuchsia deps (via download_fuchsia_deps gclient arg) when necessary. Considering the pr/51072 is very huge, keeping these condition changes in a separate change would be cleaner. The wiki has been updated already since setting the download_fuchsia_deps gclient arg explicitly won't break the existing workflow. I also mentioned in the wiki that building and running fuchsia needed a linux box (an explicit assertion in the with_envs.py). Bug: http://b/40935282 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

zijiehe-google-com and others added 16 commits February 28, 2024 14:45

remove

3b0a191

Merge branch 'main' into main

8c72cee

Merge branch 'main' into main

354f603

env

68c57b3

fuchsia_gn_sdk

2af6427

deps

5fac7ca

Merge branch 'main' into main

3cf4cf2

deps

ce46f68

import

c815089

fuchsia_sdk_root -> fuchsia_sdk

8e8832c

using_fuchsia_sdk

140321e

arch_root

3dbf579

fuchsia_sdk_path -> fuchsia_sdk

ec1d40e

pkg: -> pkg/ ; fidl: -> fidl/

d8a3f6f

DEPS

593da97

format

0e69cb1

github-actions bot added the platform-fuchsia label Mar 7, 2024

zijiehe-google-com and others added 13 commits March 7, 2024 13:59

wrong location

bf2c1ed

format

319497d

wrong zircon location

49a33e1

Merge branch 'main' into main

d45e590

missing changes

114b9b5

Merge branch 'main' into main

058a499

deps

42cb2f2

deps

e847174

remove unnecessary gn_configs

1c86d9b

Merge branch 'main' into main

85f6638

gn-sdk

52b314b

Merge branch 'main' into main

d867460

compile

e6216a5

flutter-dashboard bot added the will affect goldens label Mar 13, 2024

zijiehe-google-com requested a review from zanderso March 13, 2024 22:51

zanderso approved these changes Mar 14, 2024

View reviewed changes

zijiehe-google-com mentioned this pull request Mar 14, 2024

Roll Dart SDK from 4c64fcc134bf to df116cff5505 (3 revisions) #51288

Closed

zijiehe-google-com added 6 commits March 14, 2024 18:46

update dart

d321a75

Merge remote-tracking branch 'upstream/main'

c71c6a7

merge

a03541e

license

3b9d2a8

licenses

f0ffbbc

unexpected change

b733e37

zijiehe-google-com removed the will affect goldens label Mar 15, 2024

bdero approved these changes Mar 15, 2024

View reviewed changes

zijiehe-google-com added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 15, 2024

zijiehe-google-com mentioned this pull request Mar 15, 2024

Roll Dart SDK from 70ca2323a702 to 406955fe6950 (1 revision) #51437

Closed

auto-submit bot merged commit 1e199ce into flutter:main Mar 15, 2024
30 checks passed

engine-flutter-autoroll mentioned this pull request Mar 15, 2024

Roll Flutter Engine from 995d8903e074 to 1e199ce6239d (1 revision) flutter/flutter#145196

Merged

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

1e199ce62 [Fuchsia] Remove the use of //build/ in fuchsia (flutter/en…

df777eb

…gine#51072)

This was referenced Mar 15, 2024

[Fuchsia] remove build/fuchsia flutter/buildroot#839

Merged

[Fuchsia] only download fuchsia deps when necessary #51439

Merged

auto-submit bot mentioned this pull request Mar 18, 2024

Reverts "[Fuchsia] only download fuchsia deps when necessary (#51439)" #51500

Merged

8 tasks

zijiehe-google-com mentioned this pull request Mar 19, 2024

Reland "[Fuchsia] only download fuchsia deps when necessary (#51439)" #51508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fuchsia] Remove the use of //build/ in fuchsia #51072

[Fuchsia] Remove the use of //build/ in fuchsia #51072

zijiehe-google-com commented Feb 28, 2024 •

edited

Loading

flutter-dashboard bot commented Mar 13, 2024

flutter-dashboard bot commented Mar 13, 2024

zanderso left a comment

zijiehe-google-com commented Mar 14, 2024

zijiehe-google-com commented Mar 15, 2024 •

edited

Loading

[Fuchsia] Remove the use of //build/ in fuchsia #51072

[Fuchsia] Remove the use of //build/ in fuchsia #51072

Conversation

zijiehe-google-com commented Feb 28, 2024 • edited Loading

Motivation of the change:

Blocking issue:

Solutions:

//build/fuchsia/ from buildroot should be removed after this change.

Pre-launch Checklist

flutter-dashboard bot commented Mar 13, 2024

flutter-dashboard bot commented Mar 13, 2024

zanderso left a comment

Choose a reason for hiding this comment

zijiehe-google-com commented Mar 14, 2024

zijiehe-google-com commented Mar 15, 2024 • edited Loading

zijiehe-google-com commented Feb 28, 2024 •

edited

Loading

zijiehe-google-com commented Mar 15, 2024 •

edited

Loading