Mark defaultTargetPlatform as constant for non-debug non-web builds. #141105

sstrickl · 2024-01-08T13:35:47Z

This PR adds the Dart VM vm:platform-const-if pragma introduced in dart-lang/sdk@57a1168875 to the defaultTargetPlatform property, allowing it to be computed as if it was a constant field in non-debug AOT builds. In particular, this means that platform-specific code executed conditionally based on this property can be tree-shaken in release builds. Note that this PR changes defaultTargetPlatform to only allow overriding via debugDefaultTargetPlatformOverride in debug builds, and makes it so that compilation throws an error if code assigns todebugDefaultTargetPlatformOverride in other build modes.

Related issue: #14233

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

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

packages/flutter/lib/src/foundation/platform.dart

sstrickl · 2024-01-16T12:51:07Z

Note that I haven't marked

I added new tests to check the change I am making, or this PR is test-exempt.

above, as I'm not sure what, if any, test would be useful for this.

flutter-dashboard · 2024-01-18T09:30:40Z

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.

packages/flutter/lib/src/foundation/platform.dart

Hixie · 2024-01-18T19:12:28Z

Is vm:platform-const-if documented anywhere?

Hixie · 2024-01-18T19:14:02Z

test-exempt: optimization

mkustermann · 2024-01-18T20:18:12Z

Is vm:platform-const-if documented anywhere?

All VM related pragmas are documented at runtime/docs/pragmas.md

Hixie · 2024-01-18T21:57:24Z

@matanlurey points out (correctly) that it'd be nice to test that in production mode we do throw for setting the debug flag here. do we have a harness for testing things in production mode? if we don't i think this probably doesn't justify the cost of adding one, but if we do, it'd be nice to add this to it.

…s made.

sstrickl · 2024-01-19T10:19:30Z

@matanlurey points out (correctly) that it'd be nice to test that in production mode we do throw for setting the debug flag here. do we have a harness for testing things in production mode? if we don't i think this probably doesn't justify the cost of adding one, but if we do, it'd be nice to add this to it.

I'll take a look around and see if I find one, and if I do then add it before merging.

sstrickl · 2024-01-22T13:40:59Z

I didn't find a harness for testing non-debug-only code when I looked around, and from Discord it seems like no one else knows of one either to point me at, so I'm going to ship this as is.

eliasyishak · 2024-01-22T14:40:07Z

Hi @sstrickl, just an fyi that this PR is causing post submit errors

I'll be investigating some more to see if your PR is causing any issues related to the observed error

…builds. (flutter/flutter#141105)

flutter/flutter@5dea6b9...3ee8ff2 2024-01-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 40c5e9ce6596 to c49989292fc1 (1 revision) (flutter/flutter#141966) 2024-01-22 engine-flutter-autoroll@skia.org Roll Packages from 129e08c to e4cbf23 (11 revisions) (flutter/flutter#141962) 2024-01-22 sstrickl@google.com Mark defaultTargetPlatform as constant for non-debug non-web builds. (flutter/flutter#141105) 2024-01-22 tessertaha@gmail.com Fix `shape` and `collapsedShape` isn't applied to `ExpansionTile`'s splash ink (flutter/flutter#141777) 2024-01-22 dacoharkes@google.com Reapply "Native assets: roll deps" (#141748) (flutter/flutter#141864) 2024-01-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 11c585f48b77 to 40c5e9ce6596 (1 revision) (flutter/flutter#141956) 2024-01-22 tessertaha@gmail.com Update `ToggleButtons`, `ExpansionPanel`, and `ExpandIcon` tests for Material 3 (flutter/flutter#141868) 2024-01-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from c485c574fb1d to 11c585f48b77 (1 revision) (flutter/flutter#141952) 2024-01-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from a7b207d5a1fe to c485c574fb1d (1 revision) (flutter/flutter#141951) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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

mkustermann · 2024-02-05T11:09:16Z

Hi @sstrickl, just an fyi that this PR is causing post submit errors

I'll be investigating some more to see if your PR is causing any issues related to the observed error

@eliasyishak To follow up here, were there any issues we need to be aware of?

sstrickl · 2024-02-05T11:45:21Z

Elias reached out to me over chat and it turned out to be a flake after further investigation, hence why no revert happened.

flutter/flutter@5dea6b9...3ee8ff2 2024-01-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 40c5e9ce6596 to c49989292fc1 (1 revision) (flutter/flutter#141966) 2024-01-22 engine-flutter-autoroll@skia.org Roll Packages from 129e08c to e4cbf23 (11 revisions) (flutter/flutter#141962) 2024-01-22 sstrickl@google.com Mark defaultTargetPlatform as constant for non-debug non-web builds. (flutter/flutter#141105) 2024-01-22 tessertaha@gmail.com Fix `shape` and `collapsedShape` isn't applied to `ExpansionTile`'s splash ink (flutter/flutter#141777) 2024-01-22 dacoharkes@google.com Reapply "Native assets: roll deps" (#141748) (flutter/flutter#141864) 2024-01-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 11c585f48b77 to 40c5e9ce6596 (1 revision) (flutter/flutter#141956) 2024-01-22 tessertaha@gmail.com Update `ToggleButtons`, `ExpansionPanel`, and `ExpandIcon` tests for Material 3 (flutter/flutter#141868) 2024-01-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from c485c574fb1d to 11c585f48b77 (1 revision) (flutter/flutter#141952) 2024-01-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from a7b207d5a1fe to c485c574fb1d (1 revision) (flutter/flutter#141951) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 8, 2024

mkustermann reviewed Jan 11, 2024

View reviewed changes

packages/flutter/lib/src/foundation/platform.dart Outdated Show resolved Hide resolved

sstrickl force-pushed the platform-const-default-target-platform branch from 9d936af to b65cb3d Compare January 12, 2024 10:33

sstrickl marked this pull request as ready for review January 16, 2024 12:41

mkustermann approved these changes Jan 17, 2024

View reviewed changes

sstrickl force-pushed the platform-const-default-target-platform branch from a831652 to d4d244b Compare January 18, 2024 09:07

Hixie reviewed Jan 18, 2024

View reviewed changes

packages/flutter/lib/src/foundation/platform.dart Outdated Show resolved Hide resolved

sstrickl added 5 commits January 19, 2024 11:17

Mark defaultTargetPlatform as constant for non-debug non-web builds.

23ff2e6

import constants for kDebugMode.

fe29b8c

Fix analyzer errors.

c0c423c

Throw a FlutterError if an attempt to modify this in non-debug mode i…

995fc55

…s made.

Remove extraneous newline.

4e1c862

sstrickl force-pushed the platform-const-default-target-platform branch from 3055ed6 to 4e1c862 Compare January 19, 2024 10:17

sstrickl merged commit 7ca4b7b into flutter:master Jan 22, 2024
59 of 60 checks passed

sstrickl deleted the platform-const-default-target-platform branch January 22, 2024 13:49

engine-flutter-autoroll mentioned this pull request Jan 22, 2024

Roll Flutter from 5dea6b994e8f to 7ca4b7b86b9d (7 revisions) flutter/packages#5953

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 22, 2024

7ca4b7b Mark defaultTargetPlatform as constant for non-debug non-web …

92ccde1

…builds. (flutter/flutter#141105)

engine-flutter-autoroll mentioned this pull request Jan 22, 2024

Roll Flutter from 5dea6b994e8f to bfa245fa3485 (8 revisions) flutter/packages#5954

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 22, 2024

7ca4b7b Mark defaultTargetPlatform as constant for non-debug non-web …

332c4c8

…builds. (flutter/flutter#141105)

engine-flutter-autoroll mentioned this pull request Jan 22, 2024

Roll Flutter from 5dea6b994e8f to 3ee8ff287a6e (9 revisions) flutter/packages#5955

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 22, 2024

7ca4b7b Mark defaultTargetPlatform as constant for non-debug non-web …

0679f15

…builds. (flutter/flutter#141105)

XilaiZhang added the cp: beta cherry pick this pull request to beta release candidate branch label Jan 22, 2024

EchoEllet mentioned this pull request Sep 5, 2024

chore: improve platform check by using constants and defaultTargetPlatform singerdmx/flutter-quill#2188

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark defaultTargetPlatform as constant for non-debug non-web builds. #141105

Mark defaultTargetPlatform as constant for non-debug non-web builds. #141105

sstrickl commented Jan 8, 2024 •

edited

Loading

sstrickl commented Jan 16, 2024

flutter-dashboard bot commented Jan 18, 2024

Hixie commented Jan 18, 2024

Hixie commented Jan 18, 2024

mkustermann commented Jan 18, 2024

Hixie commented Jan 18, 2024

sstrickl commented Jan 19, 2024

sstrickl commented Jan 22, 2024

eliasyishak commented Jan 22, 2024

mkustermann commented Feb 5, 2024

sstrickl commented Feb 5, 2024

Mark defaultTargetPlatform as constant for non-debug non-web builds. #141105

Mark defaultTargetPlatform as constant for non-debug non-web builds. #141105

Conversation

sstrickl commented Jan 8, 2024 • edited Loading

Pre-launch Checklist

sstrickl commented Jan 16, 2024

flutter-dashboard bot commented Jan 18, 2024

Hixie commented Jan 18, 2024

Hixie commented Jan 18, 2024

mkustermann commented Jan 18, 2024

Hixie commented Jan 18, 2024

sstrickl commented Jan 19, 2024

sstrickl commented Jan 22, 2024

eliasyishak commented Jan 22, 2024

mkustermann commented Feb 5, 2024

sstrickl commented Feb 5, 2024

sstrickl commented Jan 8, 2024 •

edited

Loading