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

Conversation

@chinmaygarde
Copy link
Contributor

@chinmaygarde chinmaygarde commented Jul 14, 2021

The reasons for the original split and this merge are outlined in the issue this patch set purports to fix flutter/flutter#67373.

After spending an inordinate amount of time this morning patching and cross referencing buildroot hashes in DEPS on what was supposed to be a drive-by commit (#27354), I decided to kill the buildroot for good.

Steps

To preserve history, the contents of this patch aren't squashed. The steps taken were:

  • git mv all contents at the base of the engine repo into the flutter subdirectory.
  • Using a git subtree import to import the buildroot at a directory named buildoot. The provenance of all patches in the buildroot is maintained. A separate directory is needed because subtrees cannot be imported at the source root.
  • git mv all contents from the buildroot directory to the base of the repository.
  • Move the flutter/DEPS to the base of the repository. This is where the autorollers expect the file to reside.
  • Move back sundry Github and CI files that need to be at a certain known location in the repo. These were moved in step one into the subdirectory.
  • Run tests locally.

To preserve history, this patch must not be squashed and merge commit used instead. @Hixie might have permission to do this. Otherwise, squashing changes will reauthor the imported buildroot patches under my name. We lost history during the split and I am ambivalent about preserving it during the merge. But it may potentially be useful.

FAQ

Some common questions that engine developers may have:

  • What will happen to the buildroot repository?
    • It will be archived and made read-only. The buildroot entry in DEPS is gone and there is no more need to update the buildroot and cross reference the patch in DEPS manually.
  • After an engine checkout, how will the location of source files in my checkout change?
    • They won't.
  • How will this patch affect my current checkouts?
    • The .gclient file you initially authored to checkout the engine has an entry named "name": "src/flutter",. This must be changed to "name": "src", and a gclient sync -D must be performed. This is a one time change.
    • The one time change is necessary because preserving the structure of the .gclient file would have meant breaking all DEPS autorollers. Those bots are more intractable than us humans.
  • Will this break existing autorollers?
    • No.
  • Will this break existing builders?
    • Possibly. These probably stamp the .gclient that now has the outdated path. This patch must pass all CI checks to land.
    • Cirrus gclient checkouts should not break because Cirrus stamps the .gclient using the contents of .cirrus.yml which is in this repo and has been patched.
  • Will this break existing scripts that have hardcoded paths?
    • No. Existing scripts assume the engine resides within the buildroot. The final directory structure remains identical.
  • Will I be able to use git blame to follow updates to existing source files across the buildroot and engine?
    • Yes. But, the location of files in the engine has changed to be one level up. This was done via a rename. Git tracks content and not file names. Use the --follow option to commands such as git log to track changes across renames. Files in the buildroot are in their original locations.

Fixes flutter/flutter#67373.

cbracken and others added 30 commits March 5, 2019 14:13
Adds support for building with clang static analysis enabled.
GLFW is needed for the GLFW-based Windows and Linux desktop Flutter
shells.
The initial GLFW build rules did not include adding the necessary
dependent libraries to the link step.
This fixes one issue of building engine on non-corp windows machines.
This is the right way to disable SSE3 features and above to support
early pre-Penryn iMac-s.

-msse2 which I tried to use before does not actually disable SSE3 and above.

This time I have verified that no pextrb (SSE4.1) instructions are generated
by C++ compiler by disassembling dart binary.

Note: the binary still contains some SSE4.1 instructions because BoringSSL comes
with handwritten assembly - but it check CPU features before using that assembly.

Fixes flutter/flutter#24916

(This time for real)
- Respect TERM=dumb (don't print colors)
- Kill child processes properly if the roller is stopped
- Unbuffer output for better logging
- Allow for skipping the creation of a pull request
- Other mysterious features and bug fixes!
…lutter#242)

Tracked in DX-1450. Can be removed once that issue is fixed.
Tracked in DX-387. Can be removed once that is fixed.
…#240)

Tracked in DX-1449. This can be removed one that issue is fixed.
…ter#239)

This prepares the buildroot to stamp GN targets for Fuchsia SDK defined targets.

GN templates have been created in //build/fuchsia/fuchsia.gni that
can read the JSON metadata that describes the vended SDK.

GN targets that describe all known SDK parts (as described in the SDK
manifest) can be instantiated in one shot by defining a top level fuchsia_sdk
target. This stamping needs to happen once and can be done anywhere (currenly
in //build/fuchsia/BUILD.gn). Once stamped, targets that depend on
specific Fuchsia SDK parts need to explicitly depend on them by using the
name of that part. For example, depending on the :fuchsia.images part of the
SDK will generate headers for all fidl files in that part, compile any source
libraries and link the required dynamic libraries into the executable. Unlike regular
targets, this target does not exist in a GN file. The name must be looked up
from JSON manifest. This scheme seems to work well but is not resilient
to part name collision.

So that SDK and non-SDK Fuchsia builds may co-exist till the migration is done,
the is_fuchsia_sdk GN variable and FUCHSIA_SDK preprocessor defines are
set in all Fuchsia SDK builds. This allows code to target both variants. It is
the hope that the non SDK variant will be turned down and the Flutter team
assumes ownership of this component.
…#245)

* Delete unused dart deps from flutter/DEPS file

* Remove no-op assignment
The following changes have been made:

* In //build/fuchsia/sdk.gni: fuchsia_sdk has been renamed to fuchsia_sdk_root.
* //build/fuchsia/sdk.gni is no longer implicitly imported and must be imported by GN files explicitly once the is_fuchsia check is done.
* Since SDK part names are not guaranteed to be unique, the sysroot, fidl and CC parts are each put in their own directories.
Instead, targets that have SDK and non-SDK paths must configure themselves based on the flags in //build/fuchsia/sdk.gni.
This was a pattern used by GYP and has since been abandoned in favor of conditionally adding the right sources and dependencies to GN targets. The assignment filter makes debugging linker issues due to filtered source files or dependencies extremely hard to isolate. All uses of this “feature” in the Flutter buildroot have been to disable the same. Dependencies already assume that no filters are present as other buildroots (Fuchsia) don’t define these either.
We now have BUILD.gn files for libc++ and libc++abi, and the toolchain
includes 32-bit versions of libc++ and libc++abi anyway.
Based on Chromium's usage of Fontconfig
…#480)

This is required in the latest versions of GN (which I am updating
to).
These should have been a part of 647c7f8. But the switfshader targets are apparently not built on non-x64 hosts. So running the updated GN was not failing on M1 hosts.
git-subtree-dir: buildroot
git-subtree-mainline: 60097f8
git-subtree-split: 5c61df1
@google-cla
Copy link

google-cla bot commented Jul 14, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jul 14, 2021
@chinmaygarde
Copy link
Contributor Author

Ok, as expected the LUCI recipe is mad that the DEPS file location in this repo changed. I'll see if the presubmits can be pointed to a patched recipe.

The conflicts are inevitable as commits landing after this patch was submitted for review will not be auto-mergeable. But those can be resolved manually.

The CLA bot is mad that there are multiple authors. But that is kind of the point of preserving history in buildroot files.

@chinmaygarde
Copy link
Contributor Author

Looking into patching the LUCI recipe.

@chinmaygarde
Copy link
Contributor Author

chinmaygarde commented Jul 14, 2021

The recipe updates are straightforward (I am verifying them using LED runs). Per @zanderso, there is no way to land recipe updates in lockstep with the engine. His suggestion is to land the recipe change after @christopherfujino forks for the beta. The tree will effectively be closed till this patch lands. At this point, we should also figure out one of has the permissions to bypass the squash requirement on GitHub so we aren't blocked. The steps to incorporate any patches from now till that point are straightforward so the conflicts shouldn't be an issue. Does this checklist sound good?

@zanderso
Copy link
Member

plan sgtm. seeing the conflict on ci/licenses_golden/licenses_third_party, I'm wondering if any updates to the license checker script are needed. I'm guessing not since the layout is the same, right?

@Hixie
Copy link
Contributor

Hixie commented Jul 14, 2021

This is great.

I'll enable merge commits briefly, let me know when you no longer need that enabled.

Please, once this lands, audit all the wiki pages for references to the buildroot and make sure all the instructions are up to date.

@Hixie
Copy link
Contributor

Hixie commented Jul 14, 2021

Actually looks like we already allow rebase merging; that should be sufficient, right?

@chinmaygarde
Copy link
Contributor Author

I'm wondering if any updates to the license checker script are needed.

There are no updates necessary since the directory structure is the same. I believe any updates made to the tree from here on in will show up as conflicts because the files have moved. I'll follow the steps outlined in the original comment again before committing. The tree will be blocked then anyway.

Please, once this lands, audit all the wiki pages for references to the buildroot and make sure all the instructions are up to date.

Roger. We were discussing adding a bespoke fetch configuration in depot_tools for the engine. After https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3026128 lands, developers (and the recipes) should be be able to just say fetch flutter and have their environment ready instead of having to mess with .gclient files. I'll update the wiki.

Actually looks like we already allow rebase merging; that should be sufficient, right?

You are correct. For some reason, I thought only squashed commits were allowed. I don't think I have ever used anything else. Rebased commits should be fine as well.

Since the plan sounds fine, I'll submit the recipe patch for review, verify with LED runs and commit both once the beta branch cut is done (perhaps over the weekend to minimize disruption).

@chinmaygarde
Copy link
Contributor Author

@christopherfujino says go.

@chinmaygarde
Copy link
Contributor Author

I am going to table this till I have spare cycles to work on landing and following up on this.

@chinmaygarde chinmaygarde marked this pull request as draft July 29, 2021 20:21
@CaseyHillers CaseyHillers changed the base branch from master to main November 15, 2021 18:16
@godofredoc godofredoc changed the base branch from master to main November 24, 2021 07:25
@Hixie
Copy link
Contributor

Hixie commented Feb 16, 2022

@chinmaygarde I'm going to close this for now, but I have great hopes that we will one day do this. :-)

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.

☂️Merge flutter/buildroot into flutter/engine and retire the repository.