[engine] All Linux builds default to building GLFW (X11 dep)

[jwinarske]

Yesterday 32-bit arm engine builds started failing

../../third_party/glfw/src/monitor.c -o obj/third_party/glfw/src/glfw.monitor.o
| In file included from ../../third_party/glfw/src/monitor.c:28:
| In file included from ../../third_party/glfw/src/internal.h:169:
| ../../third_party/glfw/src/x11_platform.h:36:10: fatal error: 'X11/Xlib.h' file not found
| #include <X11/Xlib.h>
|          ^~~~~~~~~~~~
| 1 error generated.

Master

Looks like glfw client has been enabled by default. This is the only scenario that does this. It should default to not building glfw client. X11 is not always available nor desirable.

Complete build log available here:
https://github.com/meta-flutter/meta-flutter/runs/4613236435?check_suite_focus=true

[darshankawar]

Labeling it for further input from the team.

/cc @stuartmorgan

[stuartmorgan]

This isn't coming from the GLFW Linux embedding, it's the engine example.

[jwinarske]

Someone checked something in two days ago that affected this. I run nightly builds and it looks like the glfw client (example) started getting built by default. This shouldn't be the case.

[stuartmorgan]

Presumably it was flutter/engine#30442

[jwinarske]

Yep.

@chinmaygarde this should not build by default. It's dependent on X11 even though glfw can be built for others. Also glfw sucks for embedded Linux as it requires lots of rework...

[jwinarske]

This is now blocking all builds - meta-flutter, AGL, and Toyota internal builds. X11 cannot be a dependency for Linux builds.

@csells
@cmc5788

[jwinarske]

Any glfw/x11 dependency should be behind --no-build-glfw-shell

[chinmaygarde]

Stuart is right. The example GLFW embedder (not the one in the shell) was not being built earlier at all. The linked patch started building it under the assumption that linux hosts will be able to build the GLFW dependency. We need to be able to distinguish between the linux hosts that can build the example and those that can't. The GLFW shell is not the same as the examples however. I can add another gn flag (say --build-embedder-examples) that can be toggled. @jwinarske: Would that work? You would have to say --no-build-embedder-examples.

[jwinarske]

@chinmaygarde Whatever we use to control it, the x11 dependency has to be disabled by default, opposed to enabled. Otherwise it's broken until fixed. If we use --build-embedder-examples then it should default to false.

When an unknown argument is passed to flutter/tools/gn it causes build to fail, which is the primary reason why the breaking change should default to false. Otherwise everything downstream has to account when the flag can be introduced. Really the best case for all the downstream deps is to use a "revert" patch workaround. Something seemingly so simple causes hours of work for many people.

[jwinarske]

@chinmaygarde is this issue going to roll to beta channel, or can that be prevented?

[cmc5788]

@chinmaygarde -- I think the issue is that the commit that made x11 a dep for all Linux builds has already rolled to beta. A flag that disables it can't be used until that flag also rolls to beta. As @jwinarske mentioned, x11 deps are breaking for our build, so in the interim between this behavior being introduced and this flag rolling to beta, we have no solution.

I'd tend to agree that x11 deps being introduced to Linux default build is a breaking change and should be cherrypicked off beta as the default behavior (and is probably also not ideal as a default generally), but if not, we'd at least need the flag available on beta as a cherrypick hotfix to unbrick our builds.

Sorry for the urgency. We'll investigate adding a customer test to guard against this kind of regression in the future. But for now this is a big problem for us.

[zanderso]

@jwinarske @cmc5788 flutter/engine#30442 is on beta channel. flutter/engine#30917 may not be ideal, but if it is sufficient to unblock you, we can get it cherry-picked to beta. I believe the next hotfix release will be next week.

[jwinarske]

@zanderso @chinmaygarde
The "proposed/merged" fix is not acceptable. It defaults to building GLFW. If GLFW had flags to determine backend then it would be usable, but GLFW is hardcoded to X11. The standard backends for embedded Linux are:

1. Wayland (commercial)
2. DRM/KMS (commercial)
3. X11/XCB (mostly non-commercial, desktop compatibility, legacy, hobby)

Because the build enables GLFW by default, we would need to track when we could use the flag verses not. Adding a "unknown" flag to flutter/tools/gn causes it to fail, opposed to be ignored. So everything downstream that does not have X11 is broken. The proper logic to unblock downstream is to disable the default build of a hardcoded (X11) GLFW. This line should default to false: https://github.com/chinmaygarde/flutter_engine/blob/02873f210a09083146647647b2df3b81afe58864/examples/examples.gni#L8

AGL reported this morning their Flutter builds are officially broken.

We need this resolved ASAP, as well as a hot fix into beta. I reported this issue 29 days ago, and I was a week late on reporting it.

@csells @cmc5788

[zanderso]

@jwinarske I'm very sorry for the trouble, but I'd like to understand better why the flag added by flutter/engine#30917 won't work for you. Are you using/building several different versions/channels of Flutter? If so, which ones and for what purposes?

flutter/engine#30946

[jwinarske]

@zanderso From master to beta, the build fails. For any channel that does not support the new flag, the build fails if the flag is set. Setting up CI to track if this commit is present so we know when we can add the flag is not acceptable. The flag that enables building GLFW needs to be disabled by default in order to unblock downstream builds.

[zanderso]

@jwinarske IIUC, if we cherry-pick flutter/engine#30917 to beta, then the flag will be consistently present on both master and beta. Sorry if my question above was unclear, but what I'd like to learn is whether there's anything about your setup that means that even if the flag is on both master and beta, you'll still be blocked.

[cmc5788]

@zanderso -- thank you for following up on this! Let me know if the following explanation makes a bit more sense;

The primary concern is that adding a flag to opt out of x11 once hot-fixed is not super practical for us since we have a number of downstream customers (such as AGL) relying on our Flutter builds that are not directly consuming a source we can adjust in a single place.

It's not a matter of adding this flag in one place; we have to advise a number of downstream customers in cases where we can't just directly step in and apply patches, in partner codebases and not just internally.

For that reason we'd really strongly prefer to see the default behavior on all channels hot-fixed to the non-broken builds as default, as opposed to having to opt into the corrected behavior and then advise to propagate flag to all downstream customer builds, even if it requires an additional commit (one-line change) now to change the default before hot-fixing. We'd prefer to see that commit if it enables the more ideal behavior for Linux customers instead of piling on.

[zanderso]

@cmc5788 That makes sense. Thanks for the explanation. It would be helpful if you can confirm that applying flutter/engine#30917 and then flutter/engine#30946 to the current beta channel resolves the issue.

[cmc5788]

@zanderso -- It looks correct; when it's on master we'll be able to run a canary build confirming.

[zanderso]

@cmc5788 Both are landed on master.

[cmc5788]

Thanks! It looks good and we'll report back on canary build status. Thanks again for your responsiveness on all of this.

[jwinarske]

@zanderso canary build on master looks good.
https://github.com/meta-flutter/meta-flutter/runs/4888483652

[cmc5788]

@jwinarske great news! if that can be cherrypicked to channels at some point we should be in good shape?

[jwinarske]

@cmc5788 Yes, now we just need the cherry picking for channels affected.

[zanderso]

FYI @iskakaushik re: customer test discussion.

@godofredoc @devoncarew @itsjustkevin for cherry-pick this needs flutter/engine@db06032 followed by flutter/engine@0ae4613

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.