-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(browseros): --gn-arg overrides for configure #1516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||
|
|
@@ -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 | ||||||
|
|
||||||
|
|
@@ -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 | ||||||
|
|
@@ -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) | ||||||
|
|
@@ -271,6 +288,7 @@ def main( | |||||
| "sign": phase_sign, | ||||||
| "package": package, | ||||||
| "upload": phase_upload, | ||||||
| "extra_gn_args": extra_gn_args, | ||||||
| } | ||||||
| try: | ||||||
| pipeline = resolve_pipeline( | ||||||
|
|
@@ -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)], | ||||||
| ) | ||||||
| ) | ||||||
|
|
@@ -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" | ||||||
| ) | ||||||
| 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}") | ||||||
|
|
@@ -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. | ||||||
|
|
||||||
|
|
@@ -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( | ||||||
|
|
@@ -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: | ||||||
|
|
@@ -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, | ||||||
| ) | ||||||
|
|
@@ -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. | ||||||
|
|
||||||
|
|
@@ -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: | ||||||
|
|
@@ -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: | ||||||
|
|
@@ -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") | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The pattern Prompt To Fix With AIThis 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: | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--show-planThe
--gn-arg has no effectwarning only fires during actual execution (afterbuild_runs()is called on line 328), but--show-planreturns on line 277 before ever reaching the banner block. A user running--preset release --from compile --gn-arg symbol_level=2 --show-planwill seeGN arg overrides: symbol_level=2in 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-planoutput would prevent confusion.Prompt To Fix With AI
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!