Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
I'm starting a first review of this pull request. I completed the review and posted feedback on this pull request. Comment You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR threads cargo features, profiles, targets, and explicit package selection into settings-schema generation during bundled-resource preparation so release jobs can better reuse build artifacts and include feature-gated settings.
Concerns
- Windows arm64 packaging runs on the same x64
windows-latest-largerunner as x64 packaging, but the new target forwarding makescargo runbuild and executegenerate_settings_schemaforaarch64-pc-windows-msvc. That target binary is not executable on the x64 runner, so the arm64 release job will fail during bundled-resource preparation. - Supplemental security pass: no security issues found in the changed command construction; shell/PowerShell invocations pass feature and target values as argv elements rather than interpolating them into an evaluated shell string.
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
| $BUNDLED_RESOURCES_DIR = "$CARGO_TARGET_OUTPUT_DIR\resources" | ||
| Write-Output "Preparing bundled resources..." | ||
| & "$WINDOWS_INSTALLER_DIR\prepare_bundled_resources.ps1" -DestinationDir "$BUNDLED_RESOURCES_DIR" -Channel "$CHANNEL" -CargoProfile "$CARGO_PROFILE" | ||
| & "$WINDOWS_INSTALLER_DIR\prepare_bundled_resources.ps1" -DestinationDir "$BUNDLED_RESOURCES_DIR" -Channel "$CHANNEL" -CargoProfile "$CARGO_PROFILE" -CargoFeatures "$FEATURES" -CargoTarget "$PLATFORM_TARGET" |
There was a problem hiding this comment.
windows-latest-large runner for --arch arm64, so passing -CargoTarget aarch64-pc-windows-msvc makes cargo run build and try to execute an arm64 generate_settings_schema binary on an x64 host. Only pass CargoTarget when the target is runnable on the current host, or leave schema generation as a host-target run for cross-compiled Windows artifacts.
fc767b1 to
03226f0
Compare
There was a problem hiding this comment.
Overview
This PR threads the bundling feature set and runnable cargo target into settings schema generation across the Linux, macOS, and Windows resource-preparation scripts, while making the schema generator package selection explicit.
Concerns
script/macos/runstill drops the selected--profilewhen--generate-schemais used, so that local path can compile the schema generator with the default dev profile even whencargo bundleused another profile.- Supplemental security pass: no command-injection or secret-handling issues found in the changed argument forwarding; shell and PowerShell invocations keep user-supplied feature/target strings as single argv entries.
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
| # Pass FEATURES through (and an empty cargo profile, since this script does | ||
| # not configure one) so that the schema generation reuses the same compiled | ||
| # artifacts as the cargo bundle invocation above. | ||
| NO_LICENSES=1 "${REPO_ROOT}/script/prepare_bundled_resources" "$WARP_APP_PATH/Contents/Resources" "$WARP_CHANNEL" "" "$FEATURES" |
There was a problem hiding this comment.
💡 [SUGGESTION] --profile is parsed above into PROFILE and forwarded to cargo bundle; passing an empty profile here makes --generate-schema --profile ... compile the schema generator in dev instead of reusing the bundle profile.
| # Pass FEATURES through (and an empty cargo profile, since this script does | |
| # not configure one) so that the schema generation reuses the same compiled | |
| # artifacts as the cargo bundle invocation above. | |
| NO_LICENSES=1 "${REPO_ROOT}/script/prepare_bundled_resources" "$WARP_APP_PATH/Contents/Resources" "$WARP_CHANNEL" "" "$FEATURES" | |
| # Pass the cargo profile and features through so schema generation can reuse | |
| # the same compiled artifacts as the cargo bundle invocation above. | |
| NO_LICENSES=1 "${REPO_ROOT}/script/prepare_bundled_resources" "$WARP_APP_PATH/Contents/Resources" "$WARP_CHANNEL" "${PROFILE:-}" "$FEATURES" |
…ling." (#9676) Reverts #9632 Will look at in the future. Currently causing dev build issues like https://github.com/warpdotdev/warp-internal/actions/runs/25187609052/job/73853657131
…arpdotdev#9632) ## Description The release bundling workflow runs `prepare_bundled_resources` after the main `cargo build`, and that script in turn invokes `cargo run --bin generate_settings_schema`. Until now we passed neither `--features` nor `--target` to this run, so cargo's resolver-v2 saw a different per-package feature unification than the main build and recompiled every dependency whose enabled features differed (`rust-embed` with `debug-embed`, `warp_core`'s `release_bundle`, sentry, jemalloc, etc.). This added significant time to release builds and pulled in extra system dependencies (e.g. protoc) for jobs that should otherwise just be packaging the prebuilt binary. There was a secondary correctness issue: the schema generator iterates settings registered via `inventory::submit!`, and `#[cfg(feature = \"...\")]`-gated settings were silently omitted from the schema in any context whose feature set didn't match the main binary. The fix threads the build's feature set (and target, where applicable) through to the schema-generator invocation, and makes package selection explicit with `-p warp`. With matching features and target, cargo reuses the existing compilation artifacts; with the same feature set, the generated schema is also faithful to what the bundled binary actually exposes. ## Testing Verified `bash -n` syntax for the modified bash scripts and parsed the modified PowerShell files via `pwsh`. Bundling, schema generation, and CI behavior will be exercised by this PR's release builds. ## Agent Mode - [x] Warp Agent Mode - This PR was created via Warp's AI Agent Mode [Conversation](https://staging.warp.dev/conversation/1bd5574f-df28-4c2e-867d-40d71ed7e827)
…ling." (warpdotdev#9676) Reverts warpdotdev#9632 Will look at in the future. Currently causing dev build issues like https://github.com/warpdotdev/warp-internal/actions/runs/25187609052/job/73853657131
…ling." (warpdotdev#9676) Reverts warpdotdev#9632 Will look at in the future. Currently causing dev build issues like https://github.com/warpdotdev/warp-internal/actions/runs/25187609052/job/73853657131
…ling." (warpdotdev#9676) Reverts warpdotdev#9632 Will look at in the future. Currently causing dev build issues like https://github.com/warpdotdev/warp-internal/actions/runs/25187609052/job/73853657131

Description
The release bundling workflow runs
prepare_bundled_resourcesafter the maincargo build, and that script in turn invokescargo run --bin generate_settings_schema. Until now we passed neither--featuresnor--targetto this run, so cargo's resolver-v2 saw a different per-package feature unification than the main build and recompiled every dependency whose enabled features differed (rust-embedwithdebug-embed,warp_core'srelease_bundle, sentry, jemalloc, etc.). This added significant time to release builds and pulled in extra system dependencies (e.g. protoc) for jobs that should otherwise just be packaging the prebuilt binary.There was a secondary correctness issue: the schema generator iterates settings registered via
inventory::submit!, and#[cfg(feature = \"...\")]-gated settings were silently omitted from the schema in any context whose feature set didn't match the main binary.The fix threads the build's feature set (and target, where applicable) through to the schema-generator invocation, and makes package selection explicit with
-p warp. With matching features and target, cargo reuses the existing compilation artifacts; with the same feature set, the generated schema is also faithful to what the bundled binary actually exposes.Testing
Verified
bash -nsyntax for the modified bash scripts and parsed the modified PowerShell files viapwsh. Bundling, schema generation, and CI behavior will be exercised by this PR's release builds.Agent Mode
Conversation