Skip to content

fix(makefile): extract setup-sandbox inline bash to script for Window…#3326

Merged
WillemJiang merged 1 commit into
bytedance:mainfrom
FallingSnowFlake:fix/setup-sandbox-windows-compatibility
May 31, 2026
Merged

fix(makefile): extract setup-sandbox inline bash to script for Window…#3326
WillemJiang merged 1 commit into
bytedance:mainfrom
FallingSnowFlake:fix/setup-sandbox-windows-compatibility

Conversation

@FallingSnowFlake
Copy link
Copy Markdown
Contributor

@FallingSnowFlake FallingSnowFlake commented May 30, 2026

Fixes #3325

Why

Because during project builds in the current Makefile, Windows is configured to use cmd as the shell via that command SHELL := cmd.exe.

Considering that other parts of the project use RUN_WITH_GIT_BASH to execute the corresponding shell, this pull request also adopts that approach for the modification.

What changed

Makefile: Replace inline bash recipe with @$(RUN_WITH_GIT_BASH) ./scripts/setup-sandbox.sh
scripts/setup-sandbox.sh: New file — contains the sandbox image pull logic with fixed config parsing
Note: current the scripts/setup-sandbox.sh greps active (uncommented) config with ^sandbox: and ^ image:, which correctly extracts the URL when configured.

Bug fix verification

make setup-sandbox now correctly invokes Git Bash and pulls the Docker image

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 30, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes make setup-sandbox failing on Windows (error 255) where SHELL := cmd.exe cannot interpret the inline bash recipe. The fix follows the existing repo convention of extracting bash recipes into scripts/*.sh invoked via $(RUN_WITH_GIT_BASH), matching how config-upgrade, serve, docker, and deploy targets are wired.

Changes:

  • Extract the setup-sandbox recipe body into a new scripts/setup-sandbox.sh.
  • Invoke the script via @$(RUN_WITH_GIT_BASH) ./scripts/setup-sandbox.sh so Windows uses Git Bash.
  • Minor logic tweak: image lookup grep now matches ^sandbox: (uncommented) instead of # sandbox:.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
Makefile Replaces inline bash recipe with a Git-Bash-aware script invocation, consistent with sibling targets.
scripts/setup-sandbox.sh New script containing the sandbox image pull logic (config grep, Apple Container/Docker pull, fallbacks).

@WillemJiang
Copy link
Copy Markdown
Collaborator

@FallingSnowFlake thanks for your contribution.
The original Makefile grep matched # sandbox: (commented config), while the new script matches ^sandbox: (uncommented). This is arguably more correct, but if any users relied on # sandbox: being parsed, this is a silent behavior change. The PR description doesn't mention this. Worth confirming this is intentional.

@WillemJiang WillemJiang added question Further information is requested reviewing The PR is in reviewing status labels May 31, 2026
@FallingSnowFlake
Copy link
Copy Markdown
Contributor Author

FallingSnowFlake commented May 31, 2026

@FallingSnowFlake thanks for your contribution. The original Makefile grep matched # sandbox: (commented config), while the new script matches ^sandbox: (uncommented). This is arguably more correct, but if any users relied on # sandbox: being parsed, this is a silent behavior change. The PR description doesn't mention this. Worth confirming this is intentional.

I have added relevant explanations regarding the file change in the PR.

I think if the sandbox configuration is commented out, it means the user has not enabled the sandbox, in which case pre‑pulling the image itself is meaningless.

Also, if following the original logic when matching the commented sandbox image, there is an image matching issue because in the default configuration we use the form # # image, which has two comment symbols. This might not be particularly friendly in terms of semantics.

Of course, if you have any other questions, we can continue the discussion. Thank you for your reply.

@WillemJiang WillemJiang merged commit d6a604d into bytedance:main May 31, 2026
5 checks passed
@WillemJiang
Copy link
Copy Markdown
Collaborator

@FallingSnowFlake, thanks for your contribution.

zhongli-sz pushed a commit to zhongli-sz/deer-flow that referenced this pull request Jun 1, 2026
Wingxxx pushed a commit to Wingxxx/deer-flow that referenced this pull request Jun 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

question Further information is requested reviewing The PR is in reviewing status

Projects

None yet

Development

Successfully merging this pull request may close these issues.

make setup-sandbox fails on Windows with error code 255

4 participants