Build starboard shared library for linux-x64x11 #4215

niranjanyardi · 2024-10-07T17:29:37Z

b/365546355

Add build configuration for building starboard shared library for linux-x64x11

Note on how to build locally using same script as CI:
chrobalt/build/gn.py -p linux-x64x11 -C devel --no-check
autoninja -C out/linux-x64x11_devel/ starboard/libstarboard.so.17

starboard/build/config/base_configuration.gni

starboard/linux/shared/platform_configuration/BUILD.gn

niranjanyardi · 2024-10-07T23:47:30Z

CI failures are unrelated and are being worked on https://b.corp.google.com/issues/372012653
FileNotFoundError: [Errno 2] No such file or directory: '/github/home/starboard-toolchains/x86_64-linux-gnu-clang-chromium-17-init-8029-g27f27d15-3/bin/clang++'

chrobalt/BUILD.gn

yell0wd0g

Non-owner LGTM with some minor nits I saw.

chrobalt/build/configs/linux-x64x11/args.gn

chrobalt/build/configs/variables.gni

oxve

Looks good from my pov

.gn

BUILD.gn

build/config/BUILDCONFIG.gn

chrobalt/build/configs/variables.gni

kaidokert

I'm fine with this going in ASAP. However - please try and gate all upstream modifications with some if(chrobalt) conditions, and Starboard code has too many "for future" things left in - remove things far more liberally until we know we need them.

andrewsavage1 · 2024-10-09T13:45:34Z

chrobalt/build/configs/chrobalt.gni

+# Get the path to the starboard implementation and include its GN
+# configuration.
+
+chrobalt = true


I recommend putting this in a declare_args so people can choose to set it and renaming to is_chrobalt to fit with the style guide

andrewsavage1 · 2024-10-09T13:46:13Z

starboard/BUILD.gn

-          "//third_party/crashpad/crashpad/client",
-          "//third_party/lz4_lib:lz4",
-        ]
+        deps += [ "//third_party/lz4_lib:lz4" ]


Why were these deps separated? Seems like unnecessary churn?

andrewsavage1 · 2024-10-09T13:46:26Z

starboard/BUILD.gn

      deps += [
        "//starboard/elf_loader:elf_loader_test_install($starboard_toolchain)",
        "//starboard/loader_app:installation_manager_test_install($starboard_toolchain)",
        "//starboard/loader_app:reset_evergreen_update_test_install($starboard_toolchain)",
      ]
-


Let's not remove this line, it's unnecessary

andrewsavage1 · 2024-10-09T13:47:54Z

starboard/stub/platform_configuration/BUILD.gn

@@ -33,10 +33,6 @@ config("platform_configuration") {
    ]
  }

-  # if (!cobalt_fastbuild && (is_debug || is_devel)) {


andrewsavage1 · 2024-10-09T13:48:04Z

.gn

+    "//starboard/build/config/sabi/BUILD.gn",
+    "//starboard/build/platform_path.gni",
+  ]
+}


Add a newline at the end of the file

andrewsavage1 · 2024-10-09T14:54:56Z

chrobalt/build/configs/initialize_variables.gni

+  starboard_toolchain = cobalt_toolchain
+}
+
+set_default_toolchain(cobalt_toolchain)


It is a bad idea to set_default_toolchain here instead of where it is set upstream in the BUILDCONFIG.gn file—right now this is called twice, and the path where build_with_separate_cobalt_toolchain = false fails.

y4vor

did initial pass

y4vor · 2024-10-09T00:53:09Z

.github/config/linux.json

@@ -13,7 +13,8 @@
        "gpu_unittests",
        "gin_unittests",
        "blink_unittests",
-        "content_shell"
+        "content_shell",
+        "starboard/libstarboard.so.17"


This doesn't sound like a typical build target name. I would have expected starboard, but not a full binary name.

BUILD.gn

chrobalt/build/configs/initialize_variables.gni

y4vor · 2024-10-09T00:59:37Z

chrobalt/build/configs/initialize_variables.gni

+
+# TODO(b/371589344): Investigate if we need cobalt_is_debug which
+# avoids overriding chrome's is_debug.
+is_devel = build_type == "devel"


Not for this change, but we may need to evaluate whether we should switch to the Chromium build flavors e.g. "release" instead of "gold".

kaidokert · 2024-10-09T16:39:21Z

build/config/BUILDCONFIG.gn

@@ -360,7 +361,9 @@ default_compiler_configs = [
  "//build/config/sanitizers:default_sanitizer_flags",
 ]

-default_compiler_configs += [ "//chrobalt/build/configs:chrobalt_config" ]
+if (chrobalt) {
+  default_compiler_configs += [ "//chrobalt/build/configs:chrobalt_config" ]


Try to manually gn format those changed files. Again, presubmits don't work for the moment.

i normally run the vscode gn formatting but i think many files the changes have not been saved or something and got pushed wrongly. i'll do a follow up cl with cleanups, probably by running gn format

kaidokert

lgtm - minor cleanups still but can land

y4vor · 2024-10-09T17:37:55Z

.gn

@@ -166,3 +166,12 @@ exec_script_whitelist =
      "//tools/grit/grit_rule.gni",
      "//tools/gritsettings/BUILD.gn",
    ]
+
+import("//chrobalt/build/configs/chrobalt.gni")
+if (chrobalt) {


why not use is_chrobalt in a similar way of is_android?

andrewsavage1 · 2024-10-09T18:35:27Z

Force merging to avoid closing open comments (for later follow up)

niranjanyardi · 2024-10-09T18:36:03Z

Will address open comments in a follow up after OOO

This reverts commit 32bc5fb. Unfortunately, this breaks the base workflow with gn gen out/chrobalt Since we have a workshop for the migration coming up this is reverted now since we cannot field a fix in a timely manner.

…" (#4245)" This reverts commit 7e79f1a.

…"" (#4247) Reverts #4245 b/365546355

niranjanyardi force-pushed the experimental/build-starboard branch from 3f20aa1 to c8927df Compare October 7, 2024 18:38

niranjanyardi added 2 commits October 7, 2024 15:50

Build starboard

882b615

Formatting changes

6ab5275

niranjanyardi force-pushed the experimental/build-starboard branch from cab2a10 to 6ab5275 Compare October 7, 2024 22:54

Remove unneeded changes

82f8181

niranjanyardi force-pushed the experimental/build-starboard branch 2 times, most recently from af772a2 to ac852c9 Compare October 7, 2024 23:11

Avoid huge diff iargs.gn formatting

ac6e3d1

niranjanyardi force-pushed the experimental/build-starboard branch from ac852c9 to ac6e3d1 Compare October 7, 2024 23:12

niranjanyardi commented Oct 7, 2024

View reviewed changes

starboard/build/config/base_configuration.gni Outdated Show resolved Hide resolved

niranjanyardi commented Oct 7, 2024

View reviewed changes

starboard/linux/shared/platform_configuration/BUILD.gn Outdated Show resolved Hide resolved

Remove unneeded gn.py changes

ec5e63e

niranjanyardi force-pushed the experimental/build-starboard branch from 3bc18e2 to ab9a2fa Compare October 7, 2024 23:26

niranjanyardi commented Oct 7, 2024

View reviewed changes

starboard/linux/shared/platform_configuration/BUILD.gn Show resolved Hide resolved

Format linux/shared/BUILD.gn

c0548c6

niranjanyardi force-pushed the experimental/build-starboard branch from ab9a2fa to c0548c6 Compare October 7, 2024 23:29

niranjanyardi marked this pull request as ready for review October 7, 2024 23:30

niranjanyardi requested a review from a team as a code owner October 7, 2024 23:30

niranjanyardi requested review from briantting, andrewsavage1, kaidokert, y4vor, yell0wd0g and oxve and removed request for briantting October 7, 2024 23:30

yell0wd0g reviewed Oct 8, 2024

View reviewed changes

chrobalt/BUILD.gn Outdated Show resolved Hide resolved

yell0wd0g reviewed Oct 8, 2024

View reviewed changes

chrobalt/build/configs/linux-x64x11/args.gn Outdated Show resolved Hide resolved

chrobalt/build/configs/variables.gni Outdated Show resolved Hide resolved

niranjanyardi force-pushed the experimental/build-starboard branch from 2923599 to 69a36e1 Compare October 8, 2024 04:32

Add is_debug to assert

76befd2

niranjanyardi requested a review from oxve October 9, 2024 01:03

niranjanyardi force-pushed the experimental/build-starboard branch 2 times, most recently from 3179d4f to 6aec684 Compare October 9, 2024 01:31

Address feedback 2

bc6bbd0

niranjanyardi force-pushed the experimental/build-starboard branch from 6aec684 to bc6bbd0 Compare October 9, 2024 01:36

oxve approved these changes Oct 9, 2024

View reviewed changes

kaidokert reviewed Oct 9, 2024

View reviewed changes

.gn Outdated Show resolved Hide resolved

kaidokert reviewed Oct 9, 2024

View reviewed changes

.gn Outdated Show resolved Hide resolved

kaidokert reviewed Oct 9, 2024

View reviewed changes

BUILD.gn Outdated Show resolved Hide resolved

kaidokert reviewed Oct 9, 2024

View reviewed changes

build/config/BUILDCONFIG.gn Outdated Show resolved Hide resolved

kaidokert reviewed Oct 9, 2024

View reviewed changes

chrobalt/build/configs/variables.gni Outdated Show resolved Hide resolved

kaidokert reviewed Oct 9, 2024

View reviewed changes

chrobalt/build/configs/variables.gni Outdated Show resolved Hide resolved

kaidokert reviewed Oct 9, 2024

View reviewed changes

chrobalt/build/configs/variables.gni Outdated Show resolved Hide resolved

kaidokert approved these changes Oct 9, 2024

View reviewed changes

Remove more unneeded code

43d9706

andrewsavage1 approved these changes Oct 9, 2024

View reviewed changes

andrewsavage1 reviewed Oct 9, 2024

View reviewed changes

y4vor reviewed Oct 9, 2024

View reviewed changes

kaidokert reviewed Oct 9, 2024

View reviewed changes

kaidokert approved these changes Oct 9, 2024

View reviewed changes

Address feedback 4

d0ecf99

y4vor reviewed Oct 9, 2024

View reviewed changes

andrewsavage1 merged commit 32bc5fb into main Oct 9, 2024
49 checks passed

andrewsavage1 deleted the experimental/build-starboard branch October 9, 2024 18:35

andrewsavage1 added a commit that referenced this pull request Oct 10, 2024

Revert "Revert "Build starboard shared library for linux-x64x11 (#4215)…

97c5ac3

…" (#4245)" This reverts commit 7e79f1a.

andrewsavage1 added a commit that referenced this pull request Oct 14, 2024

Revert "Revert "Build starboard shared library for linux-x64x11 (#4215)…

79e6b53

…"" (#4247) Reverts #4245 b/365546355

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build starboard shared library for linux-x64x11 #4215

Build starboard shared library for linux-x64x11 #4215

niranjanyardi commented Oct 7, 2024 •

edited

Loading

niranjanyardi commented Oct 7, 2024

yell0wd0g left a comment

oxve left a comment

kaidokert left a comment

andrewsavage1 Oct 9, 2024

andrewsavage1 Oct 9, 2024

andrewsavage1 Oct 9, 2024

andrewsavage1 Oct 9, 2024

andrewsavage1 Oct 9, 2024

andrewsavage1 Oct 9, 2024

y4vor left a comment

y4vor Oct 9, 2024

y4vor Oct 9, 2024

kaidokert Oct 9, 2024

niranjanyardi Oct 9, 2024 •

edited

Loading

kaidokert left a comment

y4vor Oct 9, 2024

andrewsavage1 commented Oct 9, 2024

niranjanyardi commented Oct 9, 2024

Build starboard shared library for linux-x64x11 #4215

Build starboard shared library for linux-x64x11 #4215

Conversation

niranjanyardi commented Oct 7, 2024 • edited Loading

niranjanyardi commented Oct 7, 2024

yell0wd0g left a comment

Choose a reason for hiding this comment

oxve left a comment

Choose a reason for hiding this comment

kaidokert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y4vor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niranjanyardi Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

kaidokert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsavage1 commented Oct 9, 2024

niranjanyardi commented Oct 9, 2024

niranjanyardi commented Oct 7, 2024 •

edited

Loading

niranjanyardi Oct 9, 2024 •

edited

Loading