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

Conversation

@chinmaygarde
Copy link
Member

One has to manually gn args to add this flag (usually on macOS) now.

…s/gn

One has to manually `gn args` to add this flag (usually on macOS) now.
@chinmaygarde chinmaygarde requested a review from dnfield February 8, 2024 23:28
@flutter-dashboard
Copy link

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.

@chinmaygarde chinmaygarde requested a review from zanderso February 9, 2024 21:51
@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 9, 2024
@auto-submit auto-submit bot merged commit 41daa5b into flutter:main Feb 9, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 10, 2024
@gaaclarke
Copy link
Member

@chinmaygarde why did we add this? This seems like a regression. Previously someone could test vulkan on mac os with

flutter/tools/gn --runtime-mode=debug --goma --unoptimized --no-lto \
  --mac-cpu=arm64 --enable-impeller-vulkan

Now they need

flutter/tools/gn --runtime-mode=debug --goma --unoptimized --no-lto \
  --mac-cpu=arm64 --enable-impeller-vulkan --enable-impeller-vulkan-playgrounds

Why would someone want to enable vulkan but not have playgrounds? We already have a runtime flag for enabling and disabling playgrounds.

@matanlurey
Copy link
Contributor

+1 ^

@chinmaygarde chinmaygarde deleted the enable_impeller_vulkan_playgrounds branch February 12, 2024 18:53
@chinmaygarde
Copy link
Member Author

Previously someone could test vulkan on mac os

On macOS that would still not be enough for playgrounds. Thats because this GN flag was unset on macOS by default and there was no ./tools/gn flag to enable it.

I originally thought this whole flag was unnecessary and tried to remove it but ran into validation errors. @gaaclarke apparently has fixed all of them however so this flag and the ways to enable it may be unnecessary. I'll submit a canary.

But, I still don't think this is a regression. Previously, you had to args gn to enable playgrounds on Vulkan. You can still do that. Or use a ./tools/gn arg.

@gaaclarke
Copy link
Member

Maybe I'm misremembering or maybe it was a broken flag. The opengles backend works like that where --enable-impeller-opengles unlocks the playground tests. Thanks for looking into it.

@chinmaygarde
Copy link
Member Author

Yeah, OpenGL will work fine. Its the Vulkan stuff thats weird because of this extra condition.

@chinmaygarde
Copy link
Member Author

I tried removing the flags altogether in #50561 but there are still some remaining validation errors.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants