Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/browseros/bos_build/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ browseros build --preset release --skip upload,series_patches

# Resume the tail after a failure without recompiling
browseros build --preset release --from sign_macos

# One-off GN overrides while iterating (appended last, so they win)
browseros build --preset debug --gn-arg symbol_level=2 --gn-arg dcheck_always_on=true
```

- `--skip` (and a `skip:` list in profiles) subtracts **after**
Expand All @@ -91,6 +94,15 @@ browseros build --preset release --from sign_macos
sliced, later runs stay whole. A failed universal merge resumes with
just `--arch universal --from merge_universal` — no recompiles.
CLI-only: resume is a one-off, so there is no `from:` profile key.
- `--gn-arg key=value` (repeatable, any mode) appends GN overrides
**after** the flags file and product args — last write wins, so
`configure` honors them without edits to committed `config/gn/*.gn`
files. Values are verbatim GN: bools/ints bare, strings with embedded
quotes (`--gn-arg 'target_cpu="arm64"'`). CLI-only by design — a
profile wanting different flags should use a different flags file.
Only `configure` writes args.gn: a plan that skips it (e.g.
`--from compile`) reuses the existing file untouched, including any
overrides a previous invocation wrote there.

### Modules profiles — "you own this list now"

Expand Down
58 changes: 57 additions & 1 deletion packages/browseros/bos_build/cli/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"""Build CLI - Modular build system for BrowserOS"""

import os
import re
import time
from dataclasses import dataclass, replace
from pathlib import Path
Expand Down Expand Up @@ -168,6 +169,15 @@ def main(
"-S",
help="Path to Chromium source directory",
),
gn_arg: Optional[List[str]] = typer.Option(
None,
"--gn-arg",
help="Append a GN arg to args.gn after all flags (key=value, repeatable). "
"Written last, so it wins. GN syntax applies: bools/ints are bare, "
"strings need embedded quotes (--gn-arg 'target_cpu=\"arm64\"'). "
"Per-invocation only — never persisted; use a flags file for durable "
"changes.",
),
):
"""BrowserOS Build System - Modular pipeline executor

Expand Down Expand Up @@ -205,6 +215,12 @@ def main(
show_available_modules(AVAILABLE_MODULES)
return

try:
extra_gn_args = _parse_gn_args(gn_arg)
except ValueError as e:
log_error(str(e))
raise typer.Exit(1)

has_preset = preset is not None or profile is not None
has_modules = modules is not None
# --sign/--upload given affirmatively without a preset are phase flags
Expand Down Expand Up @@ -254,6 +270,7 @@ def main(
skip=skip,
from_=from_,
chromium_src=chromium_src,
extra_gn_args=extra_gn_args,
)
if show_plan:
_print_plan(projection)
Expand All @@ -271,6 +288,7 @@ def main(
"sign": phase_sign,
"package": package,
"upload": phase_upload,
"extra_gn_args": extra_gn_args,
}
try:
pipeline = resolve_pipeline(
Expand All @@ -290,9 +308,12 @@ def main(
f"Valid: {', '.join(VALID_ARCHITECTURES)}"
)
raise typer.Exit(1)
header = ["Direct pipeline (--modules/phase flags)"]
if extra_gn_args:
header.append(f"GN arg overrides: {', '.join(extra_gn_args)}")
_print_plan(
_PlanProjection(
header=["Direct pipeline (--modules/phase flags)"],
header=header,
arch_plans=[(label_arch, pipeline)],
)
)
Expand Down Expand Up @@ -353,6 +374,13 @@ def main(
log_info(f"📍 Architecture: {summary_ctx.architecture}")
log_info(f"📍 Product: {summary_ctx.product.id}")
log_info(f"📍 Build type: {summary_ctx.build_type}")
if summary_ctx.extra_gn_args:
log_info(f"📍 GN arg overrides: {', '.join(summary_ctx.extra_gn_args)}")
if not any("configure" in run_steps for _, run_steps in runs):
log_warning(
"⚠️ --gn-arg has no effect: no run in this plan includes the "
"configure step, so args.gn is reused as-is"
)
Comment on lines +377 to +383

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No-effect warning silent during --show-plan

The --gn-arg has no effect warning only fires during actual execution (after build_runs() is called on line 328), but --show-plan returns on line 277 before ever reaching the banner block. A user running --preset release --from compile --gn-arg symbol_level=2 --show-plan will see GN arg overrides: symbol_level=2 in the plan header, with no indication that configure is absent from the sliced plan and the override is a no-op. The header lists the GN args truthfully, but a co-located note in --show-plan output would prevent confusion.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/bos_build/cli/build.py
Line: 377-383

Comment:
**No-effect warning silent during `--show-plan`**

The `--gn-arg has no effect` warning only fires during actual execution (after `build_runs()` is called on line 328), but `--show-plan` returns on line 277 before ever reaching the banner block. A user running `--preset release --from compile --gn-arg symbol_level=2 --show-plan` will see `GN arg overrides: symbol_level=2` in the plan header, with no indication that configure is absent from the sliced plan and the override is a no-op. The header lists the GN args truthfully, but a co-located note in `--show-plan` output would prevent confusion.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

log_info(f"📍 Semantic version: {summary_ctx.semantic_version}")
log_info(f"📍 Chromium version: {summary_ctx.chromium_version}")
log_info(f"📍 Build offset: {summary_ctx.browseros_build_offset}")
Expand Down Expand Up @@ -445,6 +473,7 @@ def _resolve_preset(
skip: Optional[str],
from_: Optional[str],
chromium_src: Optional[Path],
extra_gn_args: Tuple[str, ...] = (),
) -> _PlanProjection:
"""Resolve preset/profile + CLI overrides into a plan projection.

Expand Down Expand Up @@ -473,6 +502,7 @@ def _resolve_preset(
skip=skip,
from_=from_,
chromium_src=chromium_src,
extra_gn_args=extra_gn_args,
)
if build_type is not None:
raise ValueError(
Expand Down Expand Up @@ -521,6 +551,8 @@ def _resolve_preset(
header.append(f"Skip: {', '.join(switches.skip)}")
if from_ is not None:
header.append(f"From: {from_}")
if extra_gn_args:
header.append(f"GN arg overrides: {', '.join(extra_gn_args)}")

def build_runs() -> List[Tuple[Context, List[str]]]:
try:
Expand Down Expand Up @@ -548,13 +580,18 @@ def build_runs() -> List[Tuple[Context, List[str]]]:
log_info(f"✓ PRESET MODE: skip={','.join(switches.skip)}")
if from_ is not None:
log_info(f"✓ PRESET MODE: from={from_}")
if extra_gn_args:
log_info(
f"✓ PRESET MODE: gn-arg overrides={','.join(extra_gn_args)}"
)
return [
(
Context(
chromium_src=src,
architecture=run_arch,
build_type=switches.build_type,
product=switches.product,
extra_gn_args=extra_gn_args,
),
steps,
)
Expand Down Expand Up @@ -585,6 +622,7 @@ def _resolve_modules_profile(
skip: Optional[str],
from_: Optional[str],
chromium_src: Optional[Path],
extra_gn_args: Tuple[str, ...] = (),
) -> _PlanProjection:
"""Project a modules: profile through the DIRECT-mode machinery.

Expand Down Expand Up @@ -635,6 +673,8 @@ def _resolve_modules_profile(
"Modules profile (enumerated pipeline — you own this list)",
f"product={eff_product} arch={eff_arch} build_type={eff_build_type}",
]
if extra_gn_args:
header.append(f"GN arg overrides: {', '.join(extra_gn_args)}")

def build_runs() -> List[Tuple[Context, List[str]]]:
try:
Expand All @@ -644,6 +684,7 @@ def build_runs() -> List[Tuple[Context, List[str]]]:
"arch": eff_arch,
"build_type": eff_build_type,
"product": eff_product,
"extra_gn_args": extra_gn_args,
}
)
except ValueError as e:
Expand All @@ -660,6 +701,21 @@ def _parse_steps_csv(value: Optional[str]) -> Tuple[str, ...]:
return tuple(s.strip() for s in value.split(",") if s.strip())


_GN_ARG_RE = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]*=.+\Z")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Regex permits whitespace-only values

_GN_ARG_RE uses .+ for the value, which allows key= (whitespace-only string) to pass validation. GN would reject or silently misbehave on such args depending on the expected type (e.g. boolean or integer args don't accept whitespace). Changing .+ to \S.* (value must start with a non-whitespace character) would reject these before they cause a confusing gn gen error downstream.

Suggested change
_GN_ARG_RE = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]*=.+\Z")
_GN_ARG_RE = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]*=\S.*\Z")
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/bos_build/cli/build.py
Line: 704

Comment:
**Regex permits whitespace-only values**

`_GN_ARG_RE` uses `.+` for the value, which allows `key=   ` (whitespace-only string) to pass validation. GN would reject or silently misbehave on such args depending on the expected type (e.g. boolean or integer args don't accept whitespace). Changing `.+` to `\S.*` (value must start with a non-whitespace character) would reject these before they cause a confusing `gn gen` error downstream.

```suggestion
_GN_ARG_RE = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]*=\S.*\Z")
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Regex accepts \r in values

The pattern .+ (without re.DOTALL) correctly rejects embedded newlines \n, but it does match carriage return \r. A value like symbol_level=2\r passes validation and is written verbatim into args.gn, which could cause a GN parse error on Unix. Replacing .+ with [^\r\n]+ closes this gap with no change to normal usage.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros/bos_build/cli/build.py
Line: 704

Comment:
**Regex accepts `\r` in values**

The pattern `.+` (without `re.DOTALL`) correctly rejects embedded newlines `\n`, but it does match carriage return `\r`. A value like `symbol_level=2\r` passes validation and is written verbatim into `args.gn`, which could cause a GN parse error on Unix. Replacing `.+` with `[^\r\n]+` closes this gap with no change to normal usage.

How can I resolve this? If you propose a fix, please make it concise.



def _parse_gn_args(values: Optional[List[str]]) -> Tuple[str, ...]:
"""Validate --gn-arg values as GN key=value; values stay verbatim."""
args = tuple(values or ())
for value in args:
if not _GN_ARG_RE.match(value):
raise ValueError(
f"Invalid --gn-arg '{value}': expected key=value, e.g. "
'symbol_level=2 or target_cpu="arm64"'
)
return args


def _print_plan(projection: _PlanProjection) -> None:
"""Print the composed steps + required env; env values never shown."""
for line in projection.header:
Expand Down
107 changes: 107 additions & 0 deletions packages/browseros/bos_build/cli/build_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
from typer.testing import CliRunner

from bos_build.browseros import app
from bos_build.cli.build import _resolve_preset
from bos_build.core.planner import Switches, plan
from bos_build.lib.testing import MockChromium
from bos_build.lib.utils import get_platform, get_platform_arch

runner = CliRunner()
Expand Down Expand Up @@ -272,5 +274,110 @@ def test_build_type_rejected_for_switch_profiles(self):
self.assertIn("owned by the preset", combined(result))


class GnArgOptionTest(_ProfileMixin):
def test_malformed_gn_arg_rejected(self):
with scrubbed_env():
result = invoke("--preset", "debug", "--gn-arg", "bogus", "--show-plan")
self.assertNotEqual(result.exit_code, 0)
self.assertIn("bogus", combined(result))
self.assertIn("key=value", combined(result))

def test_empty_gn_arg_value_rejected(self):
with scrubbed_env():
result = invoke(
"--preset", "debug", "--gn-arg", "symbol_level=", "--show-plan"
)
self.assertNotEqual(result.exit_code, 0)
self.assertIn("symbol_level=", combined(result))

def test_help_documents_repeatable(self):
result = invoke("--help")
self.assertEqual(result.exit_code, 0, combined(result))
self.assertIn("--gn-arg", result.output)
self.assertIn("repeatable", result.output)

def test_preset_show_plan_lists_overrides(self):
with scrubbed_env():
result = invoke(
"--preset",
"release",
"--gn-arg",
"symbol_level=2",
"--gn-arg",
"dcheck_always_on=true",
"--show-plan",
)
self.assertEqual(result.exit_code, 0, combined(result))
self.assertIn(
"GN arg overrides: symbol_level=2, dcheck_always_on=true", result.output
)

def test_direct_show_plan_lists_overrides(self):
with scrubbed_env():
result = invoke(
"--modules", "clean,compile", "--gn-arg", "symbol_level=2", "--show-plan"
)
self.assertEqual(result.exit_code, 0, combined(result))
self.assertIn("GN arg overrides: symbol_level=2", result.output)

def test_modules_profile_show_plan_lists_overrides(self):
path = self._profile("modules: [clean]\n")
with scrubbed_env():
result = invoke(
"--profile", str(path), "--gn-arg", "symbol_level=2", "--show-plan"
)
self.assertEqual(result.exit_code, 0, combined(result))
self.assertIn("GN arg overrides: symbol_level=2", result.output)


class GnArgPlumbingTest(_ProfileMixin):
"""--gn-arg must reach every Context the projections construct."""

def _preset_kwargs(self, **overrides):
kwargs = dict(
preset=None,
profile=None,
product=None,
arch=None,
clean=None,
provision=None,
download=None,
sign=None,
upload=None,
build_type=None,
skip=None,
from_=None,
chromium_src=None,
extra_gn_args=("symbol_level=2",),
)
kwargs.update(overrides)
return kwargs

def test_preset_build_runs_carry_extra_gn_args(self):
with tempfile.TemporaryDirectory() as tmp:
m = MockChromium(Path(tmp))
with scrubbed_env():
projection = _resolve_preset(
**self._preset_kwargs(preset="debug", chromium_src=m.src)
)
runs = projection.build_runs()
self.assertTrue(runs)
for ctx, _steps in runs:
self.assertEqual(ctx.extra_gn_args, ("symbol_level=2",))

def test_modules_profile_build_runs_carry_extra_gn_args(self):
profile_path = self._profile("modules: [clean]\n")
with tempfile.TemporaryDirectory() as tmp:
m = MockChromium(Path(tmp))
with scrubbed_env():
projection = _resolve_preset(
**self._preset_kwargs(profile=profile_path, chromium_src=m.src)
)
runs = projection.build_runs()
self.assertTrue(runs)
for ctx, _steps in runs:
self.assertEqual(ctx.extra_gn_args, ("symbol_level=2",))


if __name__ == "__main__":
unittest.main()
3 changes: 3 additions & 0 deletions packages/browseros/bos_build/core/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ class Context:
start_time: float = 0.0
product: ProductDescriptor = field(default_factory=default_product_descriptor)
gn_flags_file: Optional[Path] = None
# Per-invocation --gn-arg overrides; configure appends them last in
# args.gn (GN last-write-wins). Never persisted to profiles.
extra_gn_args: tuple[str, ...] = ()

# Third party pins
SPARKLE_VERSION: str = "2.7.0"
Expand Down
5 changes: 5 additions & 0 deletions packages/browseros/bos_build/core/resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,22 @@ def resolve_config(cli_args: Dict[str, Any]) -> List[Context]:

product = get_product_descriptor(cli_args.get("product"))

extra_gn_args = tuple(cli_args.get("extra_gn_args") or ())

log_info(f"✓ DIRECT MODE: chromium_src={chromium_src} (cli/env)")
log_info(f"✓ DIRECT MODE: architecture={architecture} (cli/env/default)")
log_info(f"✓ DIRECT MODE: build_type={build_type} (cli/default)")
log_info(f"✓ DIRECT MODE: product={product.id} (cli/default)")
if extra_gn_args:
log_info(f"✓ DIRECT MODE: gn-arg overrides={','.join(extra_gn_args)} (cli)")

return [
Context(
chromium_src=chromium_src,
architecture=architecture,
build_type=build_type,
product=product,
extra_gn_args=extra_gn_args,
)
]

Expand Down
18 changes: 18 additions & 0 deletions packages/browseros/bos_build/core/resolver_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,24 @@ def test_unknown_product_raises(self):
resolve_config(cli_args=cli_args)
self.assertIn("Unknown build.product", str(err.exception))

def test_extra_gn_args_threaded_to_context(self):
with tempfile.TemporaryDirectory() as tmp:
m = MockChromium(Path(tmp))
cli_args = {
"chromium_src": str(m.src),
"extra_gn_args": ("symbol_level=2", "dcheck_always_on=true"),
}
contexts = resolve_config(cli_args=cli_args)
self.assertEqual(
contexts[0].extra_gn_args, ("symbol_level=2", "dcheck_always_on=true")
)

def test_extra_gn_args_default_empty(self):
with tempfile.TemporaryDirectory() as tmp:
m = MockChromium(Path(tmp))
contexts = resolve_config(cli_args={"chromium_src": str(m.src)})
self.assertEqual(contexts[0].extra_gn_args, ())


class ResolvePipelineTest(unittest.TestCase):
def test_direct_mode_requires_modules_or_flags(self):
Expand Down
10 changes: 10 additions & 0 deletions packages/browseros/bos_build/steps/setup/configure.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ def execute(self, ctx: Context) -> None:
args_content += f'\ntarget_cpu = "{ctx.architecture}"\n'
args_content += "\n".join(ctx.get_product_gn_args()) + "\n"

# Appended last so GN's last-write-wins makes them authoritative
# over the flags file and product args.
if ctx.extra_gn_args:
log_info(
f"🔧 Applying {len(ctx.extra_gn_args)} gn-arg override(s): "
f"{', '.join(ctx.extra_gn_args)}"
)
args_content += "\n# --gn-arg overrides\n"
args_content += "\n".join(ctx.extra_gn_args) + "\n"

args_file.write_text(args_content)

gn_cmd = "gn.bat" if IS_WINDOWS() else "gn"
Expand Down
Loading
Loading