[et] Add support for building Dart SDK from source

[dcharkes]

Add support for building Dart from source (for doing HHH development locally).

Supports workflow tried in:

• https://github.com/flutter/flutter/issues/156575

It would be even better if et could detect if third_party/dart is the expected version. But I'm not entirely sure how to approach that.

Maybe the cli should be a config --dart=prebuilt --dart=build --dart=build-full instead.

(FYI: I'm OOO for the next couple of weeks.)

[flutter-dashboard[bot]]

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

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[johnmccutchan]

@dcharkes et workflows should do the right thing automatically.

Some questions I have

*) Does the build-dart-from-source decision have to be made when invoking gn or can it be deferred into how ninja runs? *) What is the difference between 'building Dart' and 'building Dart SDK'?

I wonder if we should default to building Dart SDK from source from third_party/dart when using et

And then we add one flag which is "--dart-source-dir" that can be used to change which directory of dart we use.

[johnmccutchan]

Thanks @dcharkes this seems super useful.

[dcharkes]

@dcharkes et workflows should do the right thing automatically.

Hm, in that case we should detect third_party/dart is a different checkout? than what DEPS specifies? E.g. using git commands to take a look at the commit hash and if there are any changes? (The safe simple solution would be to just defaulting to build from source as you suggest.)

*) What is the difference between 'building Dart' and 'building Dart SDK'?

--no-prebuilt-dart-sdk --full-dart-sdk builds all Dart artifacts, only passing --no-prebuilt-dart-sdk builds only what is used by flutter (and I believe --full-dart-sdk without --no-prebuilt-dart-sdk is an error).

I used Dart SDK for full and Dart for non-full.

*) Does the build-dart-from-source decision have to be made when invoking gn or can it be deferred into how ninja runs?

These flags are passed to gn not to ninja.

I wonder if we should default to building Dart SDK from source from third_party/dart when using et

sg!

And then we add one flag which is "--dart-source-dir" that can be used to change which directory of dart we use.

This would prevent gclient sync invocations from failing due to edits in third_party/dart. 👍 I don't know how much plumbing would be needed to achieve this.

(P.S. I'm OOO for a couple of weeks starting shortly, so this PR will likely sit. Feel free to work on it, or land something different and close it.)

[zanderso]

I wonder if we should default to building Dart SDK from source from third_party/dart when using et

I'd recommend against this because it will slow down local engine development workflows. The engine build uses a different RBE instance than the build out of the Dart tree, and is not configured to use RBE for Dart compilation, which will result in long initial build times.

[zanderso]

An overall concern I have with this change is repeating the same mistake that we made with tools/gn in which every GN option is exposed to the user on the command line through explicit plumbing. Instead, I'd recommend adding a generic "Plumb this option to GN" flag to et.

[matanlurey]

Thanks for the contribution @dcharkes!

Some thoughts (paraphrasing above):

1. Should we support every GN flag explicitly?

No. I agree with @zanderso we should support a --gn-args (or similar): https://github.com/flutter/flutter/issues/156909 <-- Let's discuss that issue there.

2. Should the Dart SDK build from source by default?

No, for the reasons Zach outlines here: it will not be RBE compiled. If we want this feature, i.e. we believe building from source is a priority, and/or building from source by default is a priority, let's file a follow-up issue and discuss further.

3. Should the et tool handle prebuilt versus source-built Dart SDK in a more automatic manner?

Mixed. I can see parallels between --rbe and use --prebuilt-dart-sdk, that is, while they could be done entirely by passing --gn-args, we'd potentially miss on things being "good by default". That is, I'd be supportive of a more first class integration for using a source-built Dart SDK (whether the actual solution looks exactly as Daco has contributed is TBD).

[athomas]

I wonder if we should default to building Dart SDK from source from third_party/dart when using et

I'd recommend against this because it will slow down local engine development workflows. The engine build uses a different RBE instance than the build out of the Dart tree, and is not configured to use RBE for Dart compilation, which will result in long initial build times.

That's not strictly* true anymore. It was true on Goma on RBE, but there's only one instance backing Dart & Flutter's RBE reclient builds.

*: While we're using the same instance, its likely that builds out of the Flutter tree don't use exactly same configurations/inputs so we're probably not benefitting from the shared cache, yet.

[chinmaygarde]

Triage: Looks like we aren't going to move ahead with this. Closing. But please re-open if I am mistaken.

[jensjoha]

I will just comment that I ran into this recently, where the only reason I could actually debug https://github.com/flutter/flutter/issues/156713 was because I was pointed to this PR that I could then patch-in. I wasted hours running my head against the well because of what turned out to be dart changes not being compiled* --- without (at least from what I saw) any warning.

I am not married to this specific change, but the current state of affairs is not great.

* or maybe half-compiled --- I managed to get a crash about mismatching sha's at one point which was when I finally figured out that something was wrong.

[matanlurey]

I know this isn't exactly what you wanted, but we are adding support for custom GN args in et as we speak:

https://github.com/flutter/engine/pull/56228

I'll update our documentation as well to explain how the flag can be used:

https://github.com/flutter/flutter/issues/157876

[matanlurey]

In theory this is good to go as et build --config host_debug --gn-args="--no-prebuilt-dart-sdk" at HEAD.

[jensjoha]

SGTM. Thanks!