Set sb_is_modular appropriately for modular builds #1181

niranjanyardi · 2023-08-07T21:32:47Z

b/294267479
b/294230277

Modular builds are of 2 types:

evergreen
cobalt toolchain with platform native linker builds

This change consolidates the build system to have mostly 2 flags for modular builds - sb_is_modular, sb_is_evergreen

sb_is_modular : Includes common GN flags and variables related to modular and evergreen GN configurations.
sb_is_evergreen: Includes only evergreen-specific GN flags and variables
build_with_separate_cobalt_toolchain: Passed as a command line GN arg for cobalt toolchain with platform native linker builds : This flag is used very minimally in the GN code as most of the changes are governed by combinations of sb_is_modular , sb_is_evergreen

Set sb_is_modular = build_with_separate_cobalt_toolchain || sb_is_evergreen
in starboard/build/config/modular/helper_variables.gni.

Change-Id: Ie1e38b2e0487d01960db9a975899f9760bc78415

starboard/evergreen/shared/platform_configuration/configuration.gni

codecov · 2023-08-08T00:49:46Z

Codecov Report

Merging #1181 (953faab) into main (0e3e107) will increase coverage by 0.04%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1181      +/-   ##
==========================================
+ Coverage   56.53%   56.57%   +0.04%     
==========================================
  Files        1899     1900       +1     
  Lines       94300    94334      +34     
==========================================
+ Hits        53308    53365      +57     
+ Misses      40992    40969      -23

see 18 files with indirect coverage changes

y4vor

Did 1st pass.

build/toolchain/gcc_toolchain.gni

cobalt/BUILD.gn

components/update_client/BUILD.gn

starboard/BUILD.gn

base/BUILD.gn

andrewsavage1 · 2023-08-09T13:34:42Z

build/toolchain/gcc_toolchain.gni

@@ -149,7 +150,6 @@ template("gcc_toolchain") {
    toolchain_args = {
      # Populate toolchain args from the invoker.
      forward_variables_from(invoker_toolchain_args, "*")
-      build_with_separate_cobalt_toolchain = build_with_separate_cobalt_toolchain


You instead should be passing through is_modular_toolchain to propagate it to other toolchains

Why does the toolchain need the argument if we have a global variable to indicate we are building modular?

the variable is only global to the toolchain, it doesn't propagate to the other toolchains unless passed to them

i dont understand, GN normally complains if the variable is not needed/ not set . None of he tests fail in the GN step because of this.

GN normally complains if the variable is not needed/ not set

The variable is needed and is set, it's just a question of what it's set to. It works now as (I believe) it is implicitly passing the variable from the outer scope to the scope captured by toolchain_args—so it is set correctly, but brittle and it's better to be explicit about passing the same value through

See https://gn.googlesource.com/gn/+/master/docs/reference.md#toolchain-overview for more info

build/toolchain/gcc_toolchain.gni

andrewsavage1 · 2023-08-09T13:43:04Z

cobalt/BUILD.gn

@@ -79,10 +81,8 @@ group("default") {
        "//components/crx_file",
        "//components/prefs",
        "//components/update_client",
+        "//third_party/llvm-project/libunwind:unwind_evergreen",


I know this isn't directly part of this PR, but this if block should not be nested in this if (sb_is_modular) block, it should be its own conditional (for cleanliness)

i think you are maybe fighting the agreement that evergreen builds are modular.
I think this should remain here, otherwise it looks out of place

@y4vor thoughts ?

I'm not—it would just clean up the code to unnest this. sb_is_modular && sb_is_evergreen is just equivalent to sb_is_evergreen

components/update_client/BUILD.gn

starboard/build/config/BUILDCONFIG.gn

b/294267479 Set sb_is_modular = is_modular_toolchain || sb_is_evergreen in starboard/build/config/modular/helper_variables.gni. Replace instances of build_with_separate_cobalt_toolchain with sb_is_modular, sb_is_evergreen. In certain places during starboard/build/config/BUILDCONFIG.gn build_with_separate_cobalt_toolchain cannot be replaced with sb_is_modular, sb_is_evergreen Fix gn errors caused due to sb_is_modular being imported before it's defined. Change-Id: I99d89f67c354252a121ddd7f2e4c95c2f4aa2b2a

b/294230277 Change-Id: I00c3832e12f8bd012a34ec9b962e70e82cc85f99

niranjanyardi · 2023-08-11T23:47:29Z

Got resolved and merged in 2 separate PR's

niranjanyardi requested review from andrewsavage1 and y4vor August 7, 2023 21:32

niranjanyardi mentioned this pull request Aug 7, 2023

Set SB_IS_MODULAR=1 for any modular shared library #1169

Closed

niranjanyardi force-pushed the use_modular_toolchain branch 2 times, most recently from d891494 to c1078c0 Compare August 7, 2023 22:15

niranjanyardi marked this pull request as draft August 7, 2023 22:23

niranjanyardi force-pushed the use_modular_toolchain branch from c1078c0 to b618328 Compare August 7, 2023 22:49

niranjanyardi marked this pull request as ready for review August 7, 2023 23:20

niranjanyardi force-pushed the use_modular_toolchain branch from 241202c to ecaa709 Compare August 7, 2023 23:45

niranjanyardi commented Aug 7, 2023

View reviewed changes

starboard/evergreen/shared/platform_configuration/configuration.gni Outdated Show resolved Hide resolved

niranjanyardi marked this pull request as draft August 8, 2023 13:41

andrewsavage1 mentioned this pull request Aug 8, 2023

Introduce cast_starboard_api. #1177

Merged

niranjanyardi force-pushed the use_modular_toolchain branch from ecaa709 to 05cb206 Compare August 9, 2023 00:03

niranjanyardi changed the title ~~Intoduce is_modular_toolchain flag for modular builds~~ Set sb_is_modular appropriately for modular and evergreen builds Aug 9, 2023

niranjanyardi marked this pull request as ready for review August 9, 2023 00:10

niranjanyardi added the on_device label Aug 9, 2023

y4vor reviewed Aug 9, 2023

View reviewed changes

niranjanyardi force-pushed the use_modular_toolchain branch from ae51941 to 5a14b28 Compare August 9, 2023 03:05

niranjanyardi requested a review from y4vor August 9, 2023 03:14

niranjanyardi force-pushed the use_modular_toolchain branch 5 times, most recently from f98b85c to 7ec515d Compare August 9, 2023 06:06

andrewsavage1 requested changes Aug 9, 2023

View reviewed changes

niranjanyardi marked this pull request as draft August 9, 2023 17:12

niranjanyardi force-pushed the use_modular_toolchain branch 3 times, most recently from ec7b9ee to 914a5bd Compare August 10, 2023 01:01

niranjanyardi changed the title ~~Set sb_is_modular appropriately for modular and evergreen builds~~ Set sb_is_modular appropriately for modular builds Aug 10, 2023

niranjanyardi force-pushed the use_modular_toolchain branch 2 times, most recently from c15bb40 to 7d8e74a Compare August 10, 2023 01:10

niranjanyardi marked this pull request as ready for review August 10, 2023 01:19

kaidokert assigned y4vor Aug 10, 2023

niranjanyardi force-pushed the use_modular_toolchain branch from 7d8e74a to effb350 Compare August 10, 2023 01:26

niranjanyardi force-pushed the use_modular_toolchain branch from effb350 to 952ca23 Compare August 10, 2023 01:27

Set sb_is_evergreen=false for linux modular builds

953faab

b/294230277 Change-Id: I00c3832e12f8bd012a34ec9b962e70e82cc85f99

niranjanyardi force-pushed the use_modular_toolchain branch from 952ca23 to 953faab Compare August 10, 2023 01:52

niranjanyardi closed this Aug 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set sb_is_modular appropriately for modular builds #1181

Set sb_is_modular appropriately for modular builds #1181

niranjanyardi commented Aug 7, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading

y4vor left a comment

andrewsavage1 Aug 9, 2023

y4vor Aug 9, 2023

andrewsavage1 Aug 9, 2023

niranjanyardi Aug 9, 2023

andrewsavage1 Aug 11, 2023

andrewsavage1 Aug 9, 2023

niranjanyardi Aug 9, 2023

niranjanyardi Aug 9, 2023

andrewsavage1 Aug 11, 2023

niranjanyardi commented Aug 11, 2023

Set sb_is_modular appropriately for modular builds #1181

Set sb_is_modular appropriately for modular builds #1181

Conversation

niranjanyardi commented Aug 7, 2023 • edited Loading

codecov bot commented Aug 8, 2023 • edited Loading

Codecov Report

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

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

niranjanyardi commented Aug 11, 2023

niranjanyardi commented Aug 7, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading