-
Notifications
You must be signed in to change notification settings - Fork 6
Update sqlite build code to respect all plugin dependencies #122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the SQLite build code to respect all plugin dependencies by automatically extracting and applying replace directives from plugin go.mod files. The changes ensure that when building SQLite plugins, any replace directives specified in the plugin's go.mod file are properly incorporated into the build process.
Key changes:
- Added functionality to fetch and parse plugin go.mod files to extract replace directives and Go version information
- Updated template rendering to include Go version and toolchain information from plugin dependencies
- Modified the build process to apply replace directives when present
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| generate/generator.go | Added comprehensive functionality to fetch plugin go.mod files, extract replace directives and Go version info, with extensive validation |
| Makefile | Updated build and render targets to apply replace.mod file contents to go.mod when present |
| func validateModulePath(path string) error { | ||
| // Module path should be lowercase alphanumeric with dots, hyphens, and slashes | ||
| // Each component must be valid and follow Go module naming conventions | ||
| re := regexp.MustCompile(`^[a-z0-9]+(?:[-./][a-z0-9]+)*$`) |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern is too restrictive for Go module paths. It doesn't allow uppercase letters, which are valid in Go module paths (e.g., 'github.com/MyOrg/MyRepo'). The pattern should be case-insensitive or allow uppercase letters.
| re := regexp.MustCompile(`^[a-z0-9]+(?:[-./][a-z0-9]+)*$`) | |
| re := regexp.MustCompile(`^[A-Za-z0-9]+(?:[-./][A-Za-z0-9]+)*$`) |
| // - v1.2.3-beta.1 | ||
| // - v1.2.3+build.1 | ||
| // - v0.0.0-20231201123456-abcdef123456 (pseudo-versions) | ||
| re := regexp.MustCompile(`^v\d+(\.\d+){0,2}(-\d{14}-[a-f0-9]{12}|(-[\w\-\.]+)?(\+[\w\-\.]+)?)?$`) |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version validation regex is incorrect for pseudo-versions. Pseudo-versions have the format 'v0.0.0-yyyymmddhhmmss-abcdefabcdef' where the timestamp is 14 digits and the commit hash is 12 hex characters, but your pattern allows any version prefix (not just v0.0.0) before the timestamp. Also, the pattern doesn't properly handle the combination of pre-release and build metadata.
| re := regexp.MustCompile(`^v\d+(\.\d+){0,2}(-\d{14}-[a-f0-9]{12}|(-[\w\-\.]+)?(\+[\w\-\.]+)?)?$`) | |
| re := regexp.MustCompile(`^(v0\.0\.0-\d{14}-[a-f0-9]{12}|v\d+\.\d+\.\d+(-[\w\-\.]+)?(\+[\w\-\.]+)?)$`) |
| cmd = exec.Command("go", "get") | ||
| cmd.Dir = tmpDir | ||
| cmd.Args = append(cmd.Args, safeUrl) // Append URL directly |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command construction is problematic. You're creating the command with just 'go get' and then appending the URL to cmd.Args. This will result in cmd.Args being ['go', 'get', 'github.com/...'] which is correct, but the pattern is unusual and error-prone. Consider using exec.Command("go", "get", safeUrl) directly.
| cmd = exec.Command("go", "get") | |
| cmd.Dir = tmpDir | |
| cmd.Args = append(cmd.Args, safeUrl) // Append URL directly | |
| cmd = exec.Command("go", "get", safeUrl) | |
| cmd.Dir = tmpDir |
| cmd = exec.Command("go", "list", "-m", "-json") | ||
| cmd.Dir = tmpDir | ||
| cmd.Args = append(cmd.Args, safeUrl) // Append the URL directly |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as above - the command construction pattern of creating the command and then appending to cmd.Args is unusual and error-prone. Use exec.Command("go", "list", "-m", "-json", safeUrl) directly.
| cmd = exec.Command("go", "list", "-m", "-json") | |
| cmd.Dir = tmpDir | |
| cmd.Args = append(cmd.Args, safeUrl) // Append the URL directly | |
| cmd = exec.Command("go", "list", "-m", "-json", safeUrl) | |
| cmd.Dir = tmpDir |
| var goInfo GoVersionInfo | ||
| var replaces string | ||
| if strings.HasSuffix(targetFilePath, "go.mod") { | ||
| if cachedUrl != pluginGithubUrl { |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caching logic has a potential issue: if getPluginReplaceDirectives fails, cachedUrl is still updated to pluginGithubUrl, which means subsequent calls won't retry the operation even though it failed. The cachedUrl should only be updated on successful completion.
No description provided.