Add secondary check for library looking within File system if pkg-config could not locate the library #4853
Conversation
📝 WalkthroughWalkthroughThe changes add a fallback mechanism to library detection in the checkLibrary function: if Changes
Sequence Diagram(s)sequenceDiagram
participant App as checkLibrary()
participant Pkg as pkg-config
participant Shell as sh (find)
participant FS as Filesystem
Note over App,Pkg: Primary attempt
App->>Pkg: run `pkg-config --cflags <name>`
alt pkg-config returns flags
Pkg-->>App: cflags (non-empty)
Note right of App: Use pkg-config result
else pkg-config empty
Pkg-->>App: empty
Note over App,Shell: Fallback search
App->>Shell: run `sh -c "find /usr/lib* /lib* -name '<name>*.so*' -print -quit"`
Shell->>FS: traverse /usr/lib*, /lib*
FS-->>Shell: paths (if found) or nothing
Shell-->>App: stdout (paths) or empty
alt stdout non-empty
App->>App: mark installed = true
else stdout empty
App->>App: mark installed = false
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
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 |
symball
force-pushed
the
libcheck_fallback
branch
from
bc9e766 to
ff6e31d
Compare
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
v2/internal/system/system.go (1)
130-136: Consider refining the pattern to avoid overly broad matches.The pattern
name*.so*(e.g.,gtk-3*.so*) might match unintended libraries. For example, if searching for "gtk-3", it would also match "gtk-30", "gtk-300", etc. While this may be acceptable for the fallback use case, consider whether a more precise pattern likelibname*.so*would be more appropriate (since libraries typically have a "lib" prefix).🔎 Alternative pattern with lib prefix
- pattern := fmt.Sprintf("%s*.so*", name) + pattern := fmt.Sprintf("lib%s*.so*", name) // We'll use the shell to keep the globbing (/usr/lib*, /lib*) and pipeline. cmdStr := fmt.Sprintf(Note: Verify that this aligns with how library names are provided to
checkLibrary.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/internal/system/system.gowebsite/src/pages/changelog.mdx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run Go Tests (macos-latest, 1.23)
- GitHub Check: Run Go Tests (ubuntu-24.04, 1.23)
- GitHub Check: Run Go Tests (ubuntu-22.04, 1.23)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
website/src/pages/changelog.mdx (1)
26-26: LGTM! Changelog entry is clear and well-formatted.The changelog entry accurately describes the new fallback mechanism and follows the established format.
v2/internal/system/system.go (2)
4-6: LGTM! Standard library imports are appropriate.The new imports (bytes, fmt, log) are all from the standard library and necessary for the fallback implementation.
128-152: Confirmed: library names are always hardcoded and trusted.The
checkLibraryfunction is called only twice in the codebase (inv2/internal/system/system_linux.go), both with hardcoded library names:"libgtk-3"and"libwebkit". There is no risk of command injection from this usage pattern.
| // As a fallback, attempt to look for presence of the library using find | ||
| if !installed { | ||
| pattern := fmt.Sprintf("%s*.so*", name) | ||
|
|
||
| // We'll use the shell to keep the globbing (/usr/lib*, /lib*) and pipeline. | ||
| // stderr is redirected to /dev/null to match the original command. | ||
| cmdStr := fmt.Sprintf( | ||
| `find /usr/lib* /lib* -type f -name %q`, | ||
| pattern, | ||
| ) | ||
|
|
||
| cmd := exec.Command("sh", "-c", cmdStr) | ||
|
|
||
| var out bytes.Buffer | ||
| cmd.Stdout = &out | ||
|
|
||
| if err := cmd.Run(); err != nil { | ||
| log.Printf("error running sh -c: %v", err) | ||
| } | ||
|
|
||
| raw := strings.TrimSpace(out.String()) | ||
| if raw != "" { | ||
| installed = true | ||
| } | ||
| } |
There was a problem hiding this comment.
Add platform guard for Linux-only fallback.
According to the PR objectives, this fallback is intended for Linux only and does not apply to Windows or macOS. However, the code lacks a platform check and will execute on all operating systems. On Windows and macOS, the paths /usr/lib* and /lib* don't exist in the expected form, causing unnecessary command failures.
🔎 Suggested fix to add platform guard
// As a fallback, attempt to look for presence of the library using find
if !installed {
+ // Only attempt filesystem fallback on Linux
+ if runtime.GOOS != "linux" {
+ return &packagemanager.Dependency{
+ Name: "lib" + name + " ",
+ PackageName: "N/A",
+ Installed: false,
+ InstallCommand: "Install via your package manager",
+ Version: "N/A",
+ Optional: false,
+ External: false,
+ }
+ }
+
pattern := fmt.Sprintf("%s*.so*", name)
// We'll use the shell to keep the globbing (/usr/lib*, /lib*) and pipeline.Note: You'll also need to add "runtime" to the imports at the top of the file.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In v2/internal/system/system.go around lines 128 to 152, the fallback that runs
a shell find over /usr/lib* and /lib* must only run on Linux; add a platform
guard using runtime.GOOS (import "runtime") and only execute the shell+find
branch when runtime.GOOS == "linux". Leave the existing logic untouched for
other OSes, and ensure the function returns or skips the fallback early when not
linux so the find command is not invoked on Windows or macOS.
There was a problem hiding this comment.
This function is only called on Linux so your suggestion is irrelevant
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where checkLibrary is called in the codebase
echo "=== Finding checkLibrary invocations ==="
rg -n "checkLibrary" --type=go -C5
echo -e "\n=== Checking for OS-specific logic in system package ==="
# Look for runtime.GOOS checks in the system package
rg -n "runtime\.GOOS|build.*linux|build.*darwin|build.*windows" v2/internal/system/ --type=go -C3
echo -e "\n=== Looking for discover method implementation ==="
# Find the discover method that might have OS-specific dependency checking
ast-grep --pattern $'func ($$$) discover() $$$'
Length of output: 9747
🏁 Script executed:
#!/bin/bash
# Also check for any build tags in the system package files
echo "=== Checking for build tags in system package ==="
fd --type f --extension go . v2/internal/system/ --exec head -3 {}
echo -e "\n=== Looking at dependency list population ==="
# Find where Dependencies are populated
rg -n "Dependencies.*=" v2/internal/system/ --type=go -C5
Length of output: 3541
@symball, you're absolutely right, and I apologize for the incorrect suggestion.
I've verified that checkLibrary is only called from system_linux.go (lines 58-59), which has //go:build linux build constraints. This means the function will never execute on Windows or macOS, even though it's defined in the shared system.go file. The Go build system ensures the call sites are only compiled for Linux.
The runtime platform guard would be redundant. Thank you for the clarification!
🧬 Code Graph Analysis Results
[v2/internal/system/packagemanager/packagemanager.go:59-101]
func Dependencies(p PackageManager) (DependencyList, error) {
var dependencies DependencyList
for name, packages := range p.Packages() {
dependency := &Dependency{Name: name}
for _, pkg := range packages {
dependency.Optional = pkg.Optional
dependency.External = !pkg.SystemPackage
dependency.InstallCommand = p.InstallCommand(pkg)
packageavailable, err := p.PackageAvailable(pkg)
if err != nil {
return nil, err
}
if packageavailable {
dependency.Version = pkg.Version
dependency.PackageName = pkg.Name
installed, err := p.PackageInstalled(pkg)
if err != nil {
return nil, err
}
if installed {
dependency.Installed = true
dependency.Version = pkg.Version
if !pkg.SystemPackage {
dependency.Version = AppVersion(name)
}
} else {
dependency.InstallCommand = p.InstallCommand(pkg)
}
break
}
}
dependencies = append(dependencies, dependency)
}
// Sort dependencies
sort.Slice(dependencies, func(i, j int) bool {
return dependencies[i].Name < dependencies[j].Name
})
return dependencies, nil
}
[v2/internal/system/packagemanager/pm.go:16-22]
PackageManager interface {
Name() string
Packages() packagemap
PackageInstalled(pkg *Package) (bool, error)
PackageAvailable(pkg *Package) (bool, error)
InstallCommand(pkg *Package) string
}
[v2/internal/system/operatingsystem/os.go:4-9]
OS struct {
ID string
Name string
Version string
Branding string
}
[v2/internal/shell/shell.go:9-15]
Command struct {
command string
args []string
env []string
dir string
stdo, stde bytes.Buffer
}
[v2/internal/shell/shell.go:63-65]
func RunCommand(directory string, command string, args ...string) (string, string, error) {
return RunCommandWithEnv(nil, directory, command, args...)
}
✏️ Learnings added
Learnt from: symball
Repo: wailsapp/wails PR: 4853
File: v2/internal/system/system.go:128-152
Timestamp: 2026-01-04T08:01:00.038Z
Learning: In the wails repository, v2/internal/system/system.go contains shared functions like checkLibrary that are defined without build tags, but these functions are only invoked from platform-specific files (system_linux.go, system_windows.go, system_darwin.go) that have appropriate build constraints. The build tags on the calling files ensure platform-specific behavior without needing runtime checks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
v2/internal/system/system.go
Outdated
| pattern := fmt.Sprintf("%s*.so*", name) | ||
|
|
||
| // We'll use the shell to keep the globbing (/usr/lib*, /lib*) and pipeline. | ||
| // stderr is redirected to /dev/null to match the original command. |
There was a problem hiding this comment.
Fix inaccurate comment or implement stderr redirection.
Line 133 states "stderr is redirected to /dev/null to match the original command," but the code doesn't actually redirect stderr. Either update the comment to reflect the actual behavior or implement the stderr redirection.
🔎 Option 1: Update comment to match implementation
- // We'll use the shell to keep the globbing (/usr/lib*, /lib*) and pipeline.
- // stderr is redirected to /dev/null to match the original command.
+ // We'll use the shell to keep the globbing (/usr/lib*, /lib*).
cmdStr := fmt.Sprintf(🔎 Option 2: Implement stderr redirection as documented
cmd := exec.Command("sh", "-c", cmdStr)
var out bytes.Buffer
cmd.Stdout = &out
+ cmd.Stderr = nil // Discard stderr
if err := cmd.Run(); err != nil {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In v2/internal/system/system.go around line 133 the comment claims "stderr is
redirected to /dev/null" but the code doesn't perform that; either update the
comment to remove the incorrect claim or implement real stderr redirection: set
the command's Stderr to a discard writer (cmd.Stderr = io.Discard) or open
os.DevNull and assign the file (f, _ := os.OpenFile(os.DevNull, os.O_WRONLY, 0);
defer f.Close(); cmd.Stderr = f) and add the necessary import, ensuring the
comment matches the chosen behavior.
…fig could not locate the library
symball
force-pushed
the
libcheck_fallback
branch
from
ff6e31d to
9f549bc
Compare
|
1 participant
Linked with #4852
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Tested on a fresh install of Fedora 43 ARM64 within a VM. does not apply to windows or mac
Test Configuration
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.