v3: convert tool has-cc to generic tool has <target>#5533
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a generic ChangesGeneric tool availability check & build warnings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR generalizes the Wails v3 CLI tool-check command from a dedicated wails3 tool has-cc to a generic wails3 tool has <target>, while preserving special handling for cc (gcc/clang). This improves extensibility for Taskfile templates and user scripts by enabling ad-hoc checks for arbitrary tools in PATH.
Changes:
- Replaced
wails3 tool has-ccwithwails3 tool has <tool>and added special-casecclogic (gcc/clang). - Updated the Linux build assets Taskfile template to use
wails3 tool has cc. - Updated/renamed the unit test to cover the new
ToolHasentrypoint.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| v3/internal/commands/tool_docker_mounts.go | Replaces the old has-cc implementation with the new generic ToolHas + HasOptions. |
| v3/internal/commands/tool_docker_mounts_test.go | Renames and adjusts the tool availability test for the new command. |
| v3/internal/commands/build_assets/linux/Taskfile.yml | Updates template usage from has-cc to has cc. |
| v3/cmd/wails3/main.go | Switches CLI registration from has-cc to has. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+25
to
+28
| func ToolHas(opts *HasOptions) error { | ||
| DisableFooter = true | ||
| _, gccErr := exec.LookPath("gcc") | ||
| _, clangErr := exec.LookPath("clang") | ||
| if gccErr == nil || clangErr == nil { | ||
| fmt.Print("true") | ||
| } else { | ||
| fmt.Print("false") | ||
| switch opts.Tool { | ||
| case "cc": |
Comment on lines
+180
to
190
| func TestToolHas(t *testing.T) { | ||
| for _, tool := range []string{"cc", "git", "nonexistent-tool-xyz"} { | ||
| out, err := captureStdout(t, func() error { return ToolHas(&HasOptions{Tool: tool}) }) | ||
| if err != nil { | ||
| t.Fatalf("ToolHas(%q) returned error: %v", tool, err) | ||
| } | ||
| if out != "true" && out != "false" { | ||
| t.Errorf("ToolHas(%q): expected \"true\" or \"false\", got %q", tool, out) | ||
| } | ||
| } | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/internal/commands/tool_docker_mounts_test.go (1)
180-190: ⚡ Quick winAssert exact results here and avoid relying on
gitbeing installed.This only verifies that
ToolHasprints a boolean-looking string, so it would still pass if generic PATH lookup always returned"false". Usinggitalso makes the “known tool” case runner-dependent. Please make one known-tool case deterministic and assert exact"true"/"false"outputs.🧪 Example tightening for this test
func TestToolHas(t *testing.T) { - for _, tool := range []string{"cc", "git", "nonexistent-tool-xyz"} { - out, err := captureStdout(t, func() error { return ToolHas(&HasOptions{Tool: tool}) }) + exe, err := os.Executable() + if err != nil { + t.Fatalf("os.Executable: %v", err) + } + t.Setenv("PATH", filepath.Dir(exe)+string(os.PathListSeparator)+os.Getenv("PATH")) + + for _, tc := range []struct { + tool string + want string + }{ + {tool: filepath.Base(exe), want: "true"}, + {tool: "nonexistent-tool-xyz", want: "false"}, + } { + out, err := captureStdout(t, func() error { return ToolHas(&HasOptions{Tool: tc.tool}) }) if err != nil { - t.Fatalf("ToolHas(%q) returned error: %v", tool, err) + t.Fatalf("ToolHas(%q) returned error: %v", tc.tool, err) } - if out != "true" && out != "false" { - t.Errorf("ToolHas(%q): expected \"true\" or \"false\", got %q", tool, out) + if out != tc.want { + t.Errorf("ToolHas(%q): want %q, got %q", tc.tool, tc.want, out) } } + + out, err := captureStdout(t, func() error { return ToolHas(&HasOptions{Tool: "cc"}) }) + if err != nil { + t.Fatalf("ToolHas(%q) returned error: %v", "cc", err) + } + if out != "true" && out != "false" { + t.Errorf("ToolHas(%q): expected \"true\" or \"false\", got %q", "cc", out) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/commands/tool_docker_mounts_test.go` around lines 180 - 190, TestToolHas currently only checks that output looks boolean and relies on the environment having "git"; make the test deterministic by asserting exact "true" for a tool we can control and "false" for a guaranteed-missing tool: replace the ambiguous "git" case with a deterministic known-good binary (for example create a temporary executable file in the test and call ToolHas on its basename) and assert out == "true", keep the nonexistent-tool case and assert out == "false"; update TestToolHas to use captureStdout to run ToolHas(&HasOptions{Tool: <name>}) for both the temp executable and the nonexistent name and assert exact outputs, referencing TestToolHas, ToolHas, HasOptions, and captureStdout to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@v3/internal/commands/tool_docker_mounts_test.go`:
- Around line 180-190: TestToolHas currently only checks that output looks
boolean and relies on the environment having "git"; make the test deterministic
by asserting exact "true" for a tool we can control and "false" for a
guaranteed-missing tool: replace the ambiguous "git" case with a deterministic
known-good binary (for example create a temporary executable file in the test
and call ToolHas on its basename) and assert out == "true", keep the
nonexistent-tool case and assert out == "false"; update TestToolHas to use
captureStdout to run ToolHas(&HasOptions{Tool: <name>}) for both the temp
executable and the nonexistent name and assert exact outputs, referencing
TestToolHas, ToolHas, HasOptions, and captureStdout to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ee6fd68-f5ee-4cf3-a001-691184516df0
📒 Files selected for processing (4)
v3/cmd/wails3/main.gov3/internal/commands/build_assets/linux/Taskfile.ymlv3/internal/commands/tool_docker_mounts.gov3/internal/commands/tool_docker_mounts_test.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v3/internal/commands/tool_docker_mounts.go (1)
30-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd empty
opts.Toolvalidation inToolHas
Inv3/internal/commands/tool_docker_mounts.go,ToolHasfalls through to the default path callingexec.LookPath(opts.Tool); whenopts.Toolis"", this ends up callingexec.LookPath("")and should instead return a clear error.🛡️ Proposed fix
func ToolHas(opts *HasOptions) error { DisableFooter = true + if opts.Tool == "" { + return fmt.Errorf("tool name is required") + } switch opts.Tool { case "cc":🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/commands/tool_docker_mounts.go` around lines 30 - 53, ToolHas currently calls exec.LookPath with an empty string when opts.Tool == "", causing unexpected behavior; add an early validation at the start of ToolHas (and handle nil opts) that returns a clear error if opts is nil or opts.Tool is empty (e.g., return fmt.Errorf("Tool must be specified")). Place this check before the switch so functions like exec.LookPath are never called with an empty string and reference the function ToolHas and the field opts.Tool when implementing the validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@v3/internal/commands/tool_docker_mounts.go`:
- Around line 30-53: ToolHas currently calls exec.LookPath with an empty string
when opts.Tool == "", causing unexpected behavior; add an early validation at
the start of ToolHas (and handle nil opts) that returns a clear error if opts is
nil or opts.Tool is empty (e.g., return fmt.Errorf("Tool must be specified")).
Place this check before the switch so functions like exec.LookPath are never
called with an empty string and reference the function ToolHas and the field
opts.Tool when implementing the validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba4fda90-48fa-4581-a7a1-11c486bdc37d
📒 Files selected for processing (2)
v3/cmd/wails3/main.gov3/internal/commands/tool_docker_mounts.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/internal/commands/task_wrapper.go (1)
44-51: ⚡ Quick winRestore
WAILS_WARNINGS_FILEafterwrapTaskreturns.Line 49 sets process-global env state but never restores it. That can leak warnings routing across multiple
wrapTaskcalls in the same process (tests or embedded usage), violating per-invocation scoping.♻️ Proposed fix
- if f, err := os.CreateTemp("", "wails-build-warnings-*"); err == nil { - f.Close() - os.Setenv(buildwarnings.EnvVar, f.Name()) - defer buildwarnings.FlushAndPrint() - } + if f, err := os.CreateTemp("", "wails-build-warnings-*"); err == nil { + _ = f.Close() + prev, hadPrev := os.LookupEnv(buildwarnings.EnvVar) + _ = os.Setenv(buildwarnings.EnvVar, f.Name()) + defer func() { + buildwarnings.FlushAndPrint() + if hadPrev { + _ = os.Setenv(buildwarnings.EnvVar, prev) + } else { + _ = os.Unsetenv(buildwarnings.EnvVar) + } + }() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/commands/task_wrapper.go` around lines 44 - 51, The code in wrapTask sets the process-wide environment variable buildwarnings.EnvVar (WAILS_WARNINGS_FILE) but never restores the prior value, leaking state across invocations; modify the wrapTask logic around the os.CreateTemp block to capture the previous environment value (e.g., os.LookupEnv(buildwarnings.EnvVar)) before calling os.Setenv, then defer a restore that either os.Setenv back to the previous value if present or os.Unsetenv if it was absent, while keeping the existing deferred buildwarnings.FlushAndPrint() behavior; reference the environment variable symbol buildwarnings.EnvVar and the wrapTask scope when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@v3/internal/commands/task_wrapper.go`:
- Around line 44-51: The code in wrapTask sets the process-wide environment
variable buildwarnings.EnvVar (WAILS_WARNINGS_FILE) but never restores the prior
value, leaking state across invocations; modify the wrapTask logic around the
os.CreateTemp block to capture the previous environment value (e.g.,
os.LookupEnv(buildwarnings.EnvVar)) before calling os.Setenv, then defer a
restore that either os.Setenv back to the previous value if present or
os.Unsetenv if it was absent, while keeping the existing deferred
buildwarnings.FlushAndPrint() behavior; reference the environment variable
symbol buildwarnings.EnvVar and the wrapTask scope when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39edc0e5-688c-445a-b5fa-54a0c00d6f3b
📒 Files selected for processing (6)
docs/src/content/docs/guides/cli.mdxv3/internal/buildwarnings/buildwarnings.gov3/internal/buildwarnings/buildwarnings_test.gov3/internal/commands/task.gov3/internal/commands/task_wrapper.gov3/internal/commands/tool_docker_mounts.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/internal/commands/tool_docker_mounts_test.go (1)
187-197: ⚡ Quick winAdd test case for whitespace-only tool argument.
The PR objectives state that validation should "return an error when the tool argument is empty or only whitespace." The test currently only covers the empty string case (line 196), but should also verify that whitespace-only inputs like
" "or" \t "produce an error.✅ Suggested additional test case
{tool: stubName + "|nonexistent-wails-tool-xyz", want: "true"}, // first alternative found {tool: "", wantErr: true}, // missing arg: error + {tool: " ", wantErr: true}, // whitespace-only: error }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/commands/tool_docker_mounts_test.go` around lines 187 - 197, Add a test case to cover whitespace-only "tool" inputs: extend the tests slice in tool_docker_mounts_test.go (the tests variable) to include an entry where tool is a whitespace-only string (e.g., " " or "\t ") and wantErr is true, so the test verifies that validate/processing of the tool argument (the logic exercised by the existing table-driven test for tool) returns an error for whitespace-only inputs just like the empty-string case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@v3/internal/commands/tool_docker_mounts_test.go`:
- Around line 187-197: Add a test case to cover whitespace-only "tool" inputs:
extend the tests slice in tool_docker_mounts_test.go (the tests variable) to
include an entry where tool is a whitespace-only string (e.g., " " or "\t ")
and wantErr is true, so the test verifies that validate/processing of the tool
argument (the logic exercised by the existing table-driven test for tool)
returns an error for whitespace-only inputs just like the empty-string case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43d21fb5-6143-4ebc-8129-16fd2b04931b
📒 Files selected for processing (2)
v3/internal/commands/tool_docker_mounts.gov3/internal/commands/tool_docker_mounts_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/internal/commands/tool_docker_mounts.go
Replaces `wails3 tool has-cc` with `wails3 tool has cc` — a generic subcommand that accepts the capability name as a positional argument. Easier to extend for future capability checks without adding new subcommands. - Rename HasCCOptions → HasOptions with pos:"1" Tool field - Rename ToolHasCC → ToolHas, dispatch on opts.Tool with error on unknown - Update main.go registration from "has-cc" to "has" - Update linux Taskfile template to use `wails3 tool has cc` - Update test to cover both the cc case and unknown-tool error path
Instead of erroring on unrecognised tool names, do a plain exec.LookPath so `wails3 tool has git`, `wails3 tool has node`, etc. all work without needing explicit cases. "cc" keeps its special logic (gcc OR clang).
Restores `wails3 tool has-cc` as a thin wrapper around `ToolHas` so
existing scripts and Taskfiles outside the generated templates continue
to work. The implementation simply calls ToolHas(&HasOptions{Tool: "cc"}).
Replace the hardcoded "cc" alias with a generic pipe-separated syntax: `wails3 tool has gcc|clang` checks for gcc OR clang in PATH. - Remove the "cc" switch case; split on "|" and LookPath each alternative - Update has-cc alias to forward to gcc|clang instead of cc - Update Taskfile template to use `wails3 tool has gcc|clang` - Update descriptions and tests accordingly
Introduces internal/buildwarnings: a lightweight file-based channel that
lets subprocess tool commands (e.g. wails3 tool has-cc) append warnings
that are then printed together at the end of the build.
How it works:
- wrapTask() creates a temp file and sets WAILS_WARNINGS_FILE in the
process environment before running any task. Child processes inherit it.
- buildwarnings.Add(source, message) appends a tab-separated record to
the file. No-ops when WAILS_WARNINGS_FILE is unset.
- buildwarnings.FlushAndPrint() reads, pretty-prints, and removes the file.
Called in wrapTask's defer (success path) and in fatal() before os.Exit
(failure path), so warnings surface regardless of build outcome.
ToolHasCC now calls buildwarnings.Add("tool has-cc", "...") before
forwarding to ToolHas, so any project still using wails3 tool has-cc
will see a deprecation notice at the end of their next build.
Docs: add tool has, tool has-cc (deprecated), tool lipo, tool capabilities,
and tool docker-mounts to the CLI reference.
…cutable
Address Copilot review comments:
- Return an error when the tool argument is empty or whitespace-only,
so `wails3 tool has` (missing arg) fails clearly rather than
silently printing "false" and exiting 0.
- Rewrite TestToolHas to create a stub executable in a temp dir and
prepend it to PATH, so every assertion checks the exact output
("true"/"false") against a controlled environment rather than
relying on whatever tools happen to be installed on the host.
os.Setenv is process-wide; without a restore the env var leaks across back-to-back wrapTask calls (visible in tests). Capture the prior value with os.LookupEnv before setting and restore (Setenv/Unsetenv) in the same deferred closure that calls FlushAndPrint.
The empty-Tool check was already added; extend the same condition to cover a nil opts pointer so ToolHas never reaches opts.Tool on a nil receiver.
The alias check previously only asserted out1 == out2 without asserting
the actual value — still the same pattern flagged by the review comment.
Add a gcc stub to the same temp dir so both ToolHasCC and
ToolHas("gcc|clang") are guaranteed to output exactly "true", regardless
of what the host has installed.
leaanthony
force-pushed
the
feat/tool-has-command
branch
from
6c0a4e1 to
094b08f
Compare
leaanthony
added a commit
that referenced
this pull request
Jun 14, 2026
#5600) * fix(v3/ci): quote pipe in 'wails3 tool has gcc|clang' Taskfile command The shell in Task's `sh:` var interprets the bare `|` as a pipe, running `wails3 tool has gcc` and piping its output to `clang`. clang exits 1 with "no input files", failing all Linux template builds. Fix: wrap the argument in shell double-quotes so the pipe is passed literally to the Go program, which splits on `|` to check each tool. Regressed in #5533 (0464978). * fix(v3/generator): disable workspace mode when loading packages for binding generation Go 1.25 changed behaviour for modules nested inside a Go workspace but not listed as workspace members: packages.Load returns the package with errors, leaving TypesInfo.Instances empty. The binding generator then finds zero services, produces no output, and the frontend vite build fails with 'Cannot find module ../bindings/<pkg>'. Fix: set GOWORK=off in the packages.Config.Env for both ResolvePatterns and LoadPackages. The binding generator analyses source code in isolation and never needs workspace-level module routing; the module's own go.mod replace directives are still honoured when GOWORK=off. Fixes all Test Templates CI failures on ubuntu/macos/windows. * fix(v3/ci): register generated template project in workspace before build Go 1.25 rejects packages in a module that is nested under the workspace root but not listed in go.work — packages.Load returns errors that leave TypesInfo.Instances empty, causing the bindings generator to find zero services and produce no frontend/bindings/ output. The template test step creates a new project inside the repo tree (which is a Go workspace), so the project module is nested but unlisted. Fix by running 'go work use .' in the generated project directory before 'wails3 build'. This registers the project as a proper workspace member so Go 1.25 loads its packages correctly and bindings are generated. Revert the earlier GOWORK=off approach in load.go — disabling workspace mode in the generator would break local development workflows where go.work ties v3 and webview2 changes together. * ci: re-trigger CI run (artifact unavailable after re-run)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wails3 tool has-ccwith the genericwails3 tool has <target>ccretains its special logic (checks forgccorclang)PATH, sowails3 tool has git,wails3 tool has node, etc. work out of the box without new subcommandswails3 tool has-cc→wails3 tool has ccTest plan
wails3 tool has ccprintstrueon a machine with gcc/clang,falsewithoutwails3 tool has gitprintstrueon a machine with git in PATHwails3 tool has nonexistent-toolprintsfalseTestToolHascovers cc, a known tool (git), and a nonexistent toolSummary by CodeRabbit
New Features
Refactor
tool has git/tool has gcc|clang); old single-tool check is deprecated as an alias.Tests
Documentation