#128 Merged at 491253f
Repository
chromiumembedded
Branch
master
Author
  1. santosh mahto
Reviewers
Description
  • Move X11 dependency under compile time flag.

Comments (33)

  1. santosh mahto author

    This PR has two commits, one for moving X11 dependency and another for actual MUS support implementation.

  2. Marshall Greenblatt

    Please see comments inline. Also run the tools/fix_style.sh script to fix style issues.

  3. santosh mahto author

    I have addressed comments in this patch. Could you looks again. Regarding manifest, I have used the older manifest file with name changed and kept under libcef/common/service_manifests.

  4. santosh mahto author

    Hi Marshall, The patch has been quite cleaned up, it would be great if you could review again

  5. santosh mahto author

    @magreenblatt : Apologies if you are very busy, but I wanted this private PR to get reviewed , It would be great if you could look again anytime soon.

  6. Marshall Greenblatt

    Thank you for your patience with the review process. This PR is looking pretty good. Have you tested it with a normal Linux build to make sure nothing has broken there?

    Once the merge conflicts and comments are addressed I think we can merge this to master. I'm working on a Chromium update currently so there might be one more round of merge conflict resolution before that point.

  7. santosh mahto author

    Thanks for showing interest to merge it, I have addressed all your comments , rebased with latest cef to avoid conflicts, and also did some more cleanup, like removing extra manifests. For all the patchset I have been testing with linux build on x11 to avoid any regression, I think it would be good to have final look from again

  8. santosh mahto author

    @magreenblatt : I revisited the patch and updated it as per latest CEF, since the manifest stuffs are already applied with chromium rebase, now this patch requires lesser changes. With this changes Ozone build (with use_ozone) and X11 (use_x11/default) builds are seperated by compile time flag. As of now ozone build works but running CEF for different ozone will work once external window mode is merged in chromium which is being seperately developed by igalia folks (here)and now is in alpha testing.

    Locally I tested this patch with ozone-wayland-dev branch and I can launch CEF for wayland and x11 platform seperately and I verified that it doesn't break any default CEF X11.

  9. santoshmahto

    I have updated this long pending patch, which was blocked due to chromium official repository content layer doesn’t supported ozone without Mus, Now Mus has been deprecated

    and content module supports ozone build for different platform, which allows to directly build CEF with this patch for different ozone platform.

    @magreenblatt : I would appreciate if you could review it and take it forward.

  10. santosh mahto author

    @magreenblatt : updated the code which now generate cef_config.h exposing the CEF build configuration. This cef_config.h could be used to expose build related configuration to application. Let me know if anything need to be taken care.

  11. Marshall Greenblatt

    Overall looks good. Please add /include/cef_config.h to the .gitignore file. Also see comments inline.

  12. Heinrich Fink

    We have a use-case where an X11-less version of CEF would be really useful. So I did some testing of this branch (rebased on master) in the last couple of days, with some help from Santosh, and ran into a few minor build issues, which I think we should address.

    1. tests/shared/browser/main_message_loop_external_pump_linux.cc has a dependency on glib.h. Since the ozone-path doesn’t pull in gtk-2, this test would fail to compile. I was able to solve this by simply adding glib-2 as a dependency explicitly.
    2. Running cefsimple in debug mode reached an assertion in window_view.cc, which I believe shouldn’t be necessary? Simply commenting that assertion out works with for ozone-enabled builds.
    3. The newly pulled in ui_controls_factory_ozone.cc caused linker errors. I had to pull in a utility class from mus to fix this. Do we want that, or just remove those test files all together? (see inline comment as well).
    4. Finally, there are a few issues with makeDistrib.py for ozone-enabled builds. For those I don’t have a good fix due to my lack of knowledge of CEF (any suggestions?):

      1. the generated cef_config.h doesn’t seem to get copied, breaking builds of e.g. cefsimple inside the generated distribution.
      2. apparently the generated distribution bundle still attempts to build cefclient as well, which of course fails now since excluded from the main tree when not using X11.

    Finally, thanks for putting all this work into this, being able to use CEF w/o X11 is really useful. I would love to see this being merged back (and maybe released?), happy to help where I can!

  13. santoshmahto

    @magreenblatt : I rebased path with latest and resolved build issue observed in latest. Would you like to look at it again

  14. Marshall Greenblatt

    Please see some minor style issues commented inline.

    Testing instructions from issue #2296:

    1. Build CEF with the following GN_DEFINES: use_ozone=true use_intel_minigbm=true (varies for gpu type)
    2. Run as: $ ./out/Release/cefsimple --use-views --ozone-platform=wayland

    @santoshmahto1906 Can you attach a screenshot to issue #2296 so we can see how cefsimple looks when running in this mode? Is views required, or can OSR also be used with this build (provided the client implements the proper opengl handling)?

    @hfink_daqri Are all of your build issues resolved?

    1. Heinrich Fink

      Still seeing this error (as mentioned above):

      ../../cef/tests/shared/browser/main_message_loop_external_pump_linux.cc:11:10: fatal error: 'glib.h' file not found
      #include <glib.h>
               ^~~~~~~~
      1 error generated.
      
      1. Heinrich Fink

        After working around the glib issue with the commit I linked above, I am running into this linker error:

        builduser@d47d47260efa ~/c/c/c/src> ninja -j14 -C out/Release_GN_x64/ cef cefsimple                           │···················
        ninja: Entering directory `out/Release_GN_x64/'                                                               │···················
        [1/1] Regenerating ninja files                                                                                │···················
        [439/440] LINK ./ceftests                                                                                     │···················
        FAILED: ceftests                                                                                              │···················
        python "../../build/toolchain/gcc_link_wrapper.py" --output="./ceftests" -- ../../third_party/llvm-build/Relea│···················
        se+Asserts/bin/clang++ -fPIC -Wl,-z,noexecstack -Wl,-z,relro -Wl,-z,now -Wl,-z,defs -Wl,--as-needed -fuse-ld=l│···················
        ld -Wl,--icf=all -Wl,--color-diagnostics -m64 -Werror -Wl,-O2 -Wl,--gc-sections -rdynamic -nostdlib++ -pie -Wl│···················
        ,--disable-new-dtags -Wl,-rpath=\$ORIGIN -o "./ceftests" -Wl,--start-group @"./ceftests.rsp" ./libcef.so -Wl,-│···················
        -end-group   -ldl -lpthread -lrt -lglib-2.0 -latomic                                                          │···················
        ld.lld: error: undefined symbol: XSetErrorHandler                                                             │···················
        >>> referenced by run_all_unittests.cc                                                                        │···················
        >>>               obj/cef/ceftests/run_all_unittests.o:(main)                                                 │···················
                                                                                                                      │···················
        ld.lld: error: undefined symbol: XSetIOErrorHandler                                                           │···················
        >>> referenced by run_all_unittests.cc                                                                        │···················
        >>>               obj/cef/ceftests/run_all_unittests.o:(main)  
        

        Looks like tests/ceftests/run_all_unittests.cc uses XSetErrorHandler in an unconditional way

  15. santoshmahto

    I have updated the PR with latest commit which address the issue faced by hfink, build and tested locally, looks fine. I am attaching screenshot to issue #2296.

    @hfink_daqri : I have fixed those build issue in ceftests in latest commit to this PR. Thanks for reporting.

    @magreenblatt : View mode is required for ozone build to work and OSR mode will not help here since as of now some bits of OSR files depends on X11 types(cursor, font related, see render_widget_host_view_osr_linux.cc) which is put under flag for ozone build.

    1. Heinrich Fink

      thanks, santosh. I can confirm that I can now build cef without the aforementioned errors. I went one step further, creating a package via executing ./make_distrib.sh --ninja-build --x64-build (it's in chromium/src/cef/tools, also see these instructions). While creating the package succeeds (after executing ninja libEGL.so in the respective build-dirs before, some how the packaging step needs those libs), there are still issues building the sources / tests that are accompanying the binary package itself. If I cd into the distribution package, execute cmake -H. -Bbuild -G Ninja -DCMAKE_BUILD_TYPE=Release, then ninja -C build, I get errors during the attempt of compiling cefclient. I believe, for ozone-enabled packages, cefclient shouldn’t be built at all, should it? Can we exclude this in the ozone-enabled package? Here is the output of the build error: pastebin-link.

      1. Marshall Greenblatt

        You bring up a good point here. We probably shouldn’t include cefclient in this distribution.

  16. santoshmahto

    @magreenblatt : The PR is up to date handling all current issues. Is it possible for you to look again in this?

  17. santoshmahto

    @Marshall Greenblatt : I updated the patch to fix distribution issue, Now binary distribution for ozone build is created wth --ozone options

    which excludes cefclient. So with ozone binary dist cefclient won’t be available for build.

    Could you have a look again and let me know if you have any comments.

    1. Marshall Greenblatt

      Thanks, this looks good to me (see one minor style issue inline).

      @hfink_daqri How does it look to you?

        1. santoshmahto

          @hfink_daqri Thanks for pointing out that, I was building all platform so libminigbm.so was always available to copy thus missed the issue. I added new commit with modification as suggested by you.