Reland "Build iOS unittest target in unopt builds" (#44356)" #44821

cyanglaz · 2023-08-17T19:54:08Z

The original PR was reverted in #44356

The assert flutter_dylib_time <= ios_test_lib_time, final_message check is reverted in 70dd9e4, so we are safe to land this again.

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 Hixie said 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.

…4356)" This reverts commit f35213f.

chinmaygarde · 2023-08-17T20:28:23Z

shell/platform/darwin/ios/BUILD.gn

@@ -263,7 +263,6 @@ source_set("ios_test_flutter_mrc") {
 }

 shared_library("ios_test_flutter") {
-  testonly = true


Why are we getting rid of this? It seems like a useful guard against non-test targets including test TUs.

When this flag is on, because flutter_framework is not testonly, it cannot depend on ios_test_flutter

We should not make flutter_framework depend on ios_test_flutter, we should update some higher level BUILD.gn that checks if unit tests are enabled and if so include ios_test_flutter (if ios etc.)

e.g. in //BUILD.gn something like

if (enable_unittests && is_ios) { public_deps += [ "//flutter/shell/platform/darwin/ios:ios_test_flutter" ] }

Can we add testonly= true back now?

No. Or I can make group(darwin) testonly? If that's a problem I can move the dependency to a upper level. Eventually I should be able to find a unbrella build target that is testonly up in the chain.

chinmaygarde · 2023-08-17T20:28:37Z

LGTM with nits.

dnfield · 2023-08-17T20:59:43Z

shell/platform/darwin/ios/BUILD.gn

@@ -263,7 +263,6 @@ source_set("ios_test_flutter_mrc") {
 }

 shared_library("ios_test_flutter") {
-  testonly = true


We should not make flutter_framework depend on ios_test_flutter, we should update some higher level BUILD.gn that checks if unit tests are enabled and if so include ios_test_flutter (if ios etc.)

cyanglaz · 2023-08-18T17:46:36Z

@chinmaygarde @dnfield Updated per review comments. PTAL

dnfield · 2023-08-21T17:08:39Z

shell/platform/darwin/BUILD.gn

@@ -9,6 +9,9 @@ import("//flutter/shell/platform/config.gni")
 group("darwin") {
  if (is_ios) {
    deps = [ "ios:flutter_framework" ]
+    if (is_debug) {


How about just if (enable_unittests)?

enable_unittests = current_toolchain == host_toolchain || is_fuchsia || is_mac, I will add || is_ios

cyanglaz · 2023-08-22T17:10:10Z

I moved the test to the top level unittest target, now we can keep the testonly flag

chinmaygarde · 2023-08-24T20:12:27Z

I am not sure why GitHub is confused about this. @dnfield Can you reset your review? The assert doesn't exist anymore so this is good to go.

…44821)" This reverts commit 1d703f0.

…#45346) Reverts #44821 clang-tidy failures https://ci.chromium.org/ui/p/flutter/builders/prod/Mac%20Production%20Engine%20Drone/137227/overview

…#44356)" (flutter/engine#44821)

…133810) flutter/engine@51090e3...ca513c9 2023-08-31 [email protected] Revert "Reland "Build iOS unittest target in unopt builds" (#44356)"" (flutter/engine#45346) 2023-08-31 [email protected] Lazily allocate RasterCacheItems only when caching is enabled (flutter/engine#45211) 2023-08-31 [email protected] Roll Skia from 8ff4fd208c26 to 5d08dadd2ef4 (3 revisions) (flutter/engine#45340) 2023-08-31 [email protected] Reland "Build iOS unittest target in unopt builds" (#44356)" (flutter/engine#44821) 2023-08-31 [email protected] Update comment const_finder.dart (flutter/engine#45180) 2023-08-31 [email protected] Replace an unnecessary util function with PostSync (flutter/engine#45190) 2023-08-31 [email protected] Roll Skia from cda0cfaadfd7 to 8ff4fd208c26 (3 revisions) (flutter/engine#45337) 2023-08-31 [email protected] Roll Dart SDK from 0cea73a8d3c3 to ac3bc9f6351a (4 revisions) (flutter/engine#45336) 2023-08-31 [email protected] [macOS] Link __availability_version_check (flutter/engine#45333) 2023-08-31 [email protected] Roll Skia from 8c05d5103d6b to cda0cfaadfd7 (3 revisions) (flutter/engine#45334) 2023-08-31 [email protected] Adds an --rbe option to tools/gn that works on Linux hosts (flutter/engine#45271) 2023-08-31 [email protected] Migrate VK calls of GrBackend* (flutter/engine#45325) 2023-08-31 [email protected] Roll buildroot (flutter/engine#45329) 2023-08-31 [email protected] Revert dl split (flutter/engine#45326) 2023-08-31 [email protected] Roll Skia from d113402de2ce to 8c05d5103d6b (4 revisions) (flutter/engine#45331) 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 [email protected],[email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…" (#45519) Reland #44821 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Revert "Revert "Build iOS unittest target in unopt builds" (flutter#4…

0e29c94

…4356)" This reverts commit f35213f.

github-actions bot added the platform-ios label Aug 17, 2023

cyanglaz requested a review from dnfield August 17, 2023 19:54

cyanglaz marked this pull request as ready for review August 17, 2023 19:54

chinmaygarde reviewed Aug 17, 2023

View reviewed changes

dnfield suggested changes Aug 17, 2023

View reviewed changes

review

75b5697

cyanglaz requested review from dnfield and chinmaygarde August 18, 2023 17:46

dnfield reviewed Aug 21, 2023

View reviewed changes

Chris Yang added 3 commits August 22, 2023 09:06

review

26438be

format

db6cb09

fix

01c77ef

cyanglaz requested a review from dnfield August 22, 2023 17:08

chinmaygarde approved these changes Aug 22, 2023

View reviewed changes

dnfield approved these changes Aug 31, 2023

View reviewed changes

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2023

auto-submit bot merged commit 1d703f0 into flutter:main Aug 31, 2023

cyanglaz deleted the reland_unittest_target_unopt_build branch August 31, 2023 20:08

zanderso added a commit that referenced this pull request Aug 31, 2023

Revert "Reland "Build iOS unittest target in unopt builds" (#44356)" (#…

4d28d4d

…44821)" This reverts commit 1d703f0.

zanderso mentioned this pull request Aug 31, 2023

Revert "Reland "Build iOS unittest target in unopt builds" (#44356)"" #45346

Merged

engine-flutter-autoroll mentioned this pull request Aug 31, 2023

Roll Flutter Engine from 51090e3cfe79 to ca513c999257 (15 revisions) flutter/flutter#133810

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 31, 2023

1d703f028 Reland "Build iOS unittest target in unopt builds" (flutter…

09e1caf

…#44356)" (flutter/engine#44821)

cyanglaz mentioned this pull request Sep 6, 2023

Reland "Build iOS unittest target in unopt builds" (#44356)"" (#45346)" #45519

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reland "Build iOS unittest target in unopt builds" (#44356)" #44821

Reland "Build iOS unittest target in unopt builds" (#44356)" #44821

Uh oh!

cyanglaz commented Aug 17, 2023 •

edited

Loading

Uh oh!

chinmaygarde Aug 17, 2023

Uh oh!

cyanglaz Aug 17, 2023

Uh oh!

dnfield Aug 17, 2023

Uh oh!

dnfield Aug 17, 2023

Uh oh!

dnfield Aug 21, 2023

Uh oh!

cyanglaz Aug 21, 2023

Uh oh!

chinmaygarde commented Aug 17, 2023

Uh oh!

dnfield Aug 17, 2023

Uh oh!

cyanglaz commented Aug 18, 2023

Uh oh!

dnfield Aug 21, 2023

Uh oh!

cyanglaz Aug 21, 2023

Uh oh!

cyanglaz commented Aug 22, 2023

Uh oh!

chinmaygarde commented Aug 24, 2023

Uh oh!

Uh oh!

Reland "Build iOS unittest target in unopt builds" (#44356)" #44821

Reland "Build iOS unittest target in unopt builds" (#44356)" #44821

Uh oh!

Conversation

cyanglaz commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Aug 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Aug 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Aug 22, 2023

Uh oh!

chinmaygarde commented Aug 24, 2023

Uh oh!

Uh oh!

cyanglaz commented Aug 17, 2023 •

edited

Loading