Centralize build feature profiles in Cargo metadata#11103
Conversation
github-actions
Bot
added
the
external-contributor
|
This PR is not linked to an issue that is marked with Issue-state enforcement details:
Readiness check:
To continue, link this PR to a same-repo issue such as Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is not linked to an issue that is marked with ready-to-implement.
Issue-state enforcement details:
-
Associated same-repo issues checked: #10979
-
Required readiness label:
ready-to-implement
Readiness check:
- #10979: missing
ready-to-implement; readiness labels present: none
To continue, link this PR to a same-repo issue such as Closes #123 in the PR description, and make sure that issue has ready-to-implement.
Powered by Oz
VitalyAnkh
force-pushed
the
fix/10979-build-profile-matrix
branch
2 times, most recently
from
1bfbe0f to
d2d3820
Compare
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
oz-for-oss
Bot
dismissed
their stale review
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR centralizes the packaging/build feature matrix in app/Cargo.toml metadata and updates the Nix and bundle scripts to resolve their Cargo features from that shared source. I did not find blocking correctness, security, or spec-alignment issues in the changed build paths.
Concerns
- One non-blocking robustness concern: the new profile resolver should reject non-leaf profile paths instead of treating parent objects as empty feature sets.
Verdict
Found: 0 critical, 0 important, 1 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| def resolve_profile($package; $profile_path): | ||
| ($package.metadata.warp.build_profiles | descend($profile_path | split("."))) as $profile | ||
| | if ($profile | type) != "object" then |
There was a problem hiding this comment.
💡 [SUGGESTION] Reject profile objects that lack base, groups, and features; as written, a parent path like linux is an object and resolves successfully to an empty feature list instead of reporting a mistyped/non-leaf profile.
oz-for-oss
Bot
requested review from
a team and
lucieleblanc
and removed request for
a team
VitalyAnkh
force-pushed
the
fix/10979-build-profile-matrix
branch
from
d2d3820 to
09868ce
Compare
|
Updated for the latest review suggestion.
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR centralizes packaging/build feature profiles in app/Cargo.toml and updates Nix plus bundle/run scripts to resolve features through the shared metadata helper.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| build_profile = "linux.oss.app" | ||
|
|
||
| [package.metadata.warp.build_feature_groups] | ||
| linux_app_base = [ |
There was a problem hiding this comment.
This generally makes sense, but I think we can actually cut down some of the boilerplate by getting rid of "base" groups. Most of the Cargo features here are cross-cutting, so we can put them in corresponding groups rather than repeating them across different base configurations.
For example, something like:
# Feature groups specific to the kind of artifact
app = ["release_bundle", "gui"]
cli = ["release_bundle", "standalone"]
# Feature groups for Warp-built artifacts
linux_official = ["crash_reporting"]
macos_official = ["cocoa_sentry"]
# Platform-specific features
macos_common = ["extern_plist"]That way, we don't need to list out all the combinations - linux_cli_base becomes ["cli", "linux_official"], and macos_app_base becomes ["app", "macos_official", "macos_common"]
This also simplifies adding new artifacts, as in #11772 - we can pick from the existing set of groups, instead of needing to add a bunch of new base group definitions.
There was a problem hiding this comment.
Done — I reworked the metadata to use composable cross-cutting groups (app, cli, platform/common groups, diagnostics groups, NLD groups, etc.) instead of the combination-style base groups. I also tightened the resolver so stale base entries are rejected rather than silently accepted.
| "release_bundle", | ||
| "crash_reporting", | ||
| "standalone", | ||
| "agent_mode_debug", |
There was a problem hiding this comment.
@kevinyang372 correct me if I'm wrong, but I'm pretty sure we don't need agent_mode_debug here - it should be fine to omit
There was a problem hiding this comment.
Done — windows.local.app now omits agent_debug / agent_mode_debug. I also re-checked the current base script/windows/bundle.ps1; the local channel does not append agent_mode_debug, while the dev channel still does.
| warpMetadata = appCargoToml.package.metadata.warp; | ||
| buildFeatureGroups = warpMetadata.build_feature_groups; | ||
| buildProfiles = warpMetadata.build_profiles; | ||
| nixBuildProfile = |
There was a problem hiding this comment.
Instead of defining the build profile for Nix in app/Cargo.toml, it seems more consistent to do so here, the same way that the platform-specific build scripts construct their profiles
There was a problem hiding this comment.
Done — the Nix-selected profile is now defined in flake.nix as nixBuildProfile = "linux.oss.app", matching the convention that each packaging entrypoint chooses its own profile.
| warpMetadata.nix.build_profile | ||
| else | ||
| throw "app/Cargo.toml package.metadata.warp.nix.build_profile is missing"; | ||
| uniqueFeatures = |
There was a problem hiding this comment.
Order of Cargo features shouldn't matter, so we can use lib.lists.uniqueStrings for this
There was a problem hiding this comment.
Done — replaced the custom Nix unique helper with lib.lists.uniqueStrings.
| throw "app/Cargo.toml package.metadata.warp.build_profiles.${profilePath} references undefined Cargo features: ${lib.concatStringsSep ", " missingFeatures}" | ||
| else | ||
| features; | ||
| invalidBuildProfiles = |
There was a problem hiding this comment.
The Nix flake shouldn't need to validate all profiles
There was a problem hiding this comment.
Done — flake evaluation now only resolves and validates the selected Nix profile. Full-matrix validation remains in script/check_cargo_build_profiles --all.
VitalyAnkh
force-pushed
the
fix/10979-build-profile-matrix
branch
from
09868ce to
d5b117e
Compare
|
Updated for the latest review comments. What changed:
Verified locally:
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR centralizes build feature profile resolution in Cargo metadata and updates Nix plus bundle/run scripts to consume those profiles.
Concerns
- The Windows local app profile drops
agent_mode_debug, which the previousscript/windows/bundle.ps1local channel path appended before building. That changes local Windows bundle behavior while the dev profile still keeps the debug feature.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| groups = ["cli", "macos_common"] | ||
|
|
||
| [package.metadata.warp.build_profiles.windows.local.app] | ||
| groups = ["app", "windows_official", "nld_v2"] |
There was a problem hiding this comment.
agent_debug; the old script/windows/bundle.ps1 local path appended agent_mode_debug, but this profile now omits it.
| groups = ["app", "windows_official", "nld_v2"] | |
| groups = ["app", "windows_official", "agent_debug", "nld_v2"] |
There was a problem hiding this comment.
I re-checked this against the current PR base (origin/master at ac4225c1). The base script/windows/bundle.ps1 does not append agent_mode_debug for the local channel; it only appends it in the dev branch.
Base behavior:
local: setsWARP_BIN,BINARY_NAME, andAPP_NAME, with no feature append.dev: appendsagent_mode_debug.local/dev/preview: append the NLD v2 features afterward.
The current metadata profile resolves to:
windows.local.app=release_bundle,gui,crash_reporting,nld_classifier_v2,nld_heuristic_v2
So leaving windows.local.app without agent_debug preserves the current base behavior and matches the earlier human review note that local should be fine without agent_mode_debug. Adding agent_debug here would introduce the behavior change this comment is trying to avoid.
|
I re-checked the Windows local I left the code unchanged for this point because adding /oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
oz-for-oss
Bot
dismissed
their stale review
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR centralizes packaging feature profiles in app/Cargo.toml metadata and updates the Nix, bundle, deploy, wasm, and run scripts to resolve features from that shared metadata.
Concerns
- No blocking correctness, security, or spec-drift concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
VitalyAnkh
force-pushed
the
fix/10979-build-profile-matrix
branch
from
d5b117e to
df6c389
Compare
|
Updated this branch for the current upstream What changed in this update:
Verified locally:
I did not find any new review feedback in this poll; this was a rebase/mergeability update for current upstream |
Package scripts and Nix were drifting from app/Cargo.toml feature policy, so the build matrix now resolves named metadata profiles from the manifest. The profiles use composable cross-cutting feature groups instead of platform/artifact base groups, Nix selects its own profile path, and flake evaluation only resolves that selected profile. Constraint: Rebased upstream now applies the Cargo-backed profile resolver across Linux, macOS, Windows, run, wasm, and remote-server entrypoints while preserving the current bundle scripts' effective feature matrix. Rejected: Leaving feature arrays duplicated in platform scripts | would keep the stale flag failure mode from issue warpdotdev#10979. Rejected: Adding agent_debug to windows.local.app | current upstream Windows local builds do not add it, and adding it would change local bundle behavior. Rejected: Dropping macOS stable/preview heap diagnostics | current script/macos/bundle applies heap diagnostics for those release channels. Rejected: Dropping Linux preview heap diagnostics | current script/linux/bundle applies heap diagnostics for preview artifacts. Rejected: Omitting Linux/macOS warpctrl profiles | would make the metadata resolver drop an upstream artifact path's warp_control_cli feature. Rejected: Keeping Nix's build profile in Cargo metadata | platform scripts construct their profile paths locally, and Nix follows the same selected-profile convention. Rejected: Supporting base build-profile groups in resolvers | the review asked to remove base groups, so stale base entries now fail validation. Rejected: Validating every Cargo build profile during flake evaluation | full-matrix validation belongs in script/check_cargo_build_profiles, while flake evaluation should only resolve the Nix-selected profile. Confidence: high Scope-risk: moderate Directive: When adding packaging features, add reusable package.metadata.warp.build_feature_groups and compose leaf package.metadata.warp.build_profiles from groups/features only. Tested: script/linux/test_bundle_warpctrl; script/check_cargo_build_profiles --all; cargo metadata accepted all 44 resolved profiles; targeted profile checks for Linux/macOS/Windows app profiles using nld_v3 across local/dev/preview/stable/oss, Linux dev/preview heap diagnostics, macOS dev warp_control_cli, macOS stable/preview heap diagnostics, Windows local without agent_debug, and Linux/macOS warpctrl with warp_control_cli; non-leaf and missing profile negative checks for script/check_cargo_build_profiles; nix eval .#warp-terminal-experimental.buildFeatures --json; nix eval .#warp-terminal-experimental.meta.mainProgram --raw; rg found no nld_improvements in flake.nix, app/Cargo.toml, or script; bash -n modified shell scripts; git diff --check origin/master HEAD; git diff --check; git merge-tree --write-tree origin/master HEAD; nix build .#warp-terminal-experimental --no-link --print-out-paths --option download-attempts 8 --option max-jobs 1. Not-tested: PowerShell parse on Windows because pwsh is unavailable locally.
VitalyAnkh
force-pushed
the
fix/10979-build-profile-matrix
branch
from
df6c389 to
ea9749c
Compare
|
Rebased and refreshed this branch against The main change is still the same: bundle scripts and Nix now resolve their feature lists from I fixed the Linux Local checks passed:
|
3 participants
Summary
Fixes #10979.
This centralizes the Cargo feature matrix in
app/Cargo.tomlso the bundle scripts and Nix package resolve named build profiles from the same metadata instead of keeping their own stale feature lists.package.metadata.warp.build_feature_groupsandpackage.metadata.warp.build_profilesflake.nixto read the Nix build profile from Cargo metadatascript/check_cargo_build_profilesto validate the declared profile matrixagent_mode_debug, while macOS preview stays off heap diagnosticsValidation
script/check_cargo_build_profiles --profile windows.local.appscript/check_cargo_build_profiles --profile macos.preview.appscript/check_cargo_build_profiles --profile macos.preview.cliscript/check_cargo_build_profiles --allcargo metadata --format-version 1 --no-deps --locked --no-default-features --features "$features" --manifest-path app/Cargo.tomlfor every declared profilenix eval .#warp-terminal-experimental.buildFeatures --jsonnix eval --impure --expr 'let app = builtins.fromTOML (builtins.readFile ./app/Cargo.toml); in app.package.metadata.warp.nix.build_profile' --rawnix build .#warp-terminal-experimental --no-link --print-out-pathsbash -n script/cargo_bundle_features.sh script/check_cargo_build_profiles script/deploy_remote_server script/deploy_remote_server_to_test_vm script/linux/bundle script/macos/bundle script/run script/wasm/bundlegit diff --check