crosscompile: add mquickjs JavaScript engine support#1517
crosscompile: add mquickjs JavaScript engine support#1517luoliwoshang wants to merge 5 commits intoxgo-dev:mainfrom
Conversation
Add support for compiling mquickjs as an embedded JavaScript engine for bare-metal targets. This follows the same pattern as libc and rtlib compilation. Changes: - Add mquickjs compile configuration in compile/mquickjs/mquickjs.go - Add JSLib field to targets.Config for target configuration - Add getJSLibCompileConfigByName() for jslib compilation - Integrate jslib compilation in UseTarget() To use, add "jslib": "mquickjs" to target configuration JSON. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @luoliwoshang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the capability to cross-compile the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for the mquickjs JavaScript engine, following the existing pattern for libc and rtlib compilation. The changes look good and are well-structured. I've identified a few areas for improvement regarding code duplication and maintainability. Specifically, I've suggested using constants to manage the mquickjs version information to avoid hardcoding it in multiple places. I also pointed out some code duplication in crosscompile.go and libc.go that could be refactored to improve maintainability. Overall, a solid contribution.
| // GetMquickjsConfig returns the configuration for downloading mquickjs | ||
| func GetMquickjsConfig() compile.LibConfig { | ||
| return compile.LibConfig{ | ||
| Name: "mquickjs", | ||
| Version: "v1.0.0", | ||
| Url: "https://github.com/luoliwoshang/mquickjs/archive/refs/tags/v1.0.0.tar.gz", | ||
| ResourceSubDir: "mquickjs-1.0.0", | ||
| } | ||
| } | ||
|
|
||
| // mquickjs compile flags for embedded targets | ||
| var _mquickjsCCFlags = []string{ | ||
| "-fdata-sections", | ||
| "-ffunction-sections", | ||
| "-fno-math-errno", | ||
| "-fno-trapping-math", | ||
| } | ||
|
|
||
| var _mquickjsLDFlags = []string{ | ||
| "-Wl,--gc-sections", | ||
| } | ||
|
|
||
| // GetMquickjsCompileConfig returns configuration for compiling mquickjs | ||
| func GetMquickjsCompileConfig(baseDir, target string) compile.CompileConfig { | ||
| return compile.CompileConfig{ | ||
| ExportCFlags: []string{ | ||
| "-I" + baseDir, | ||
| }, | ||
| Groups: []compile.CompileGroup{ | ||
| { | ||
| OutputFileName: fmt.Sprintf("libmquickjs-%s.a", target), | ||
| Files: []string{ | ||
| filepath.Join(baseDir, "mquickjs.c"), | ||
| filepath.Join(baseDir, "cutils.c"), | ||
| filepath.Join(baseDir, "dtoa.c"), | ||
| filepath.Join(baseDir, "libm.c"), | ||
| }, | ||
| CFlags: []string{ | ||
| "-DCONFIG_VERSION=\"1.0.0\"", | ||
| "-D_GNU_SOURCE", | ||
| "-nostdlib", | ||
| "-I" + baseDir, | ||
| }, | ||
| CCFlags: _mquickjsCCFlags, | ||
| LDFlags: _mquickjsLDFlags, | ||
| }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
The mquickjs version and related strings are hardcoded in multiple places within GetMquickjsConfig and GetMquickjsCompileConfig. This practice can lead to inconsistencies and makes updating the library version error-prone. To improve maintainability, it's better to define these values as constants at the top of the file and derive other strings from them.
const (
Name = "mquickjs"
Version = "1.0.0"
TagName = "v" + Version
)
// GetMquickjsConfig returns the configuration for downloading mquickjs
func GetMquickjsConfig() compile.LibConfig {
return compile.LibConfig{
Name: Name,
Version: TagName,
Url: fmt.Sprintf("https://github.com/luoliwoshang/mquickjs/archive/refs/tags/%s.tar.gz", TagName),
ResourceSubDir: fmt.Sprintf("%s-%s", Name, Version),
}
}
// mquickjs compile flags for embedded targets
var _mquickjsCCFlags = []string{
"-fdata-sections",
"-ffunction-sections",
"-fno-math-errno",
"-fno-trapping-math",
}
var _mquickjsLDFlags = []string{
"-Wl,--gc-sections",
}
// GetMquickjsCompileConfig returns configuration for compiling mquickjs
func GetMquickjsCompileConfig(baseDir, target string) compile.CompileConfig {
return compile.CompileConfig{
ExportCFlags: []string{
"-I" + baseDir,
},
Groups: []compile.CompileGroup{
{
OutputFileName: fmt.Sprintf("libmquickjs-%s.a", target),
Files: []string{
filepath.Join(baseDir, "mquickjs.c"),
filepath.Join(baseDir, "cutils.c"),
filepath.Join(baseDir, "dtoa.c"),
filepath.Join(baseDir, "libm.c"),
},
CFlags: []string{
"-DCONFIG_VERSION=\"" + Version + "\"",
"-D_GNU_SOURCE",
"-nostdlib",
"-I" + baseDir,
},
CCFlags: _mquickjsCCFlags,
LDFlags: _mquickjsLDFlags,
},
},
}
}| if config.JSLib != "" { | ||
| var outputDir string | ||
| var jslibLDFlags []string | ||
| var compileConfig compile.CompileConfig | ||
| baseDir := filepath.Join(cacheRoot(), "crosscompile") | ||
|
|
||
| outputDir, compileConfig, err = getJSLibCompileConfigByName(baseDir, config.JSLib, config.LLVMTarget) | ||
| if err != nil { | ||
| return | ||
| } | ||
| jslibLDFlags, err = compileWithConfig(compileConfig, outputDir, compile.CompileOptions{ | ||
| CC: export.CC, | ||
| Linker: export.Linker, | ||
| CCFLAGS: ccflags, | ||
| LDFLAGS: ldflags, | ||
| CFLAGS: libcIncludeDir, | ||
| }) | ||
| if err != nil { | ||
| return | ||
| } | ||
| cflags = append(cflags, compileConfig.ExportCFlags...) | ||
| ldflags = append(ldflags, jslibLDFlags...) | ||
| } |
There was a problem hiding this comment.
The logic for compiling JSLib is nearly identical to the existing logic for Libc (lines 598-622) and RTLib (lines 624-645). This duplication makes the code harder to maintain. I recommend refactoring this repeated logic into a single helper function. This function could accept parameters like the library name and a closure to retrieve the specific compile configuration, which would handle the differences between getLibcCompileConfigByName, getRTCompileConfigByName, and getJSLibCompileConfigByName.
| func getJSLibCompileConfigByName(baseDir, jslibName, target string) (outputDir string, cfg compile.CompileConfig, err error) { | ||
| if jslibName == "" { | ||
| err = fmt.Errorf("jslib name cannot be empty") | ||
| return | ||
| } | ||
| var jslibDir string | ||
| var config compile.LibConfig | ||
| var compileConfig compile.CompileConfig | ||
|
|
||
| switch jslibName { | ||
| case "mquickjs": | ||
| config = mquickjs.GetMquickjsConfig() | ||
| jslibDir = filepath.Join(baseDir, config.String()) | ||
| compileConfig = mquickjs.GetMquickjsCompileConfig(jslibDir, target) | ||
| default: | ||
| err = fmt.Errorf("unsupported jslib: %s", jslibName) | ||
| return | ||
| } | ||
| if needSkipDownload { | ||
| return jslibDir, compileConfig, err | ||
| } | ||
|
|
||
| if err = checkDownloadAndExtractLib(config.Url, jslibDir, config.ResourceSubDir); err != nil { | ||
| return | ||
| } | ||
|
|
||
| return jslibDir, compileConfig, nil | ||
| } |
There was a problem hiding this comment.
The function signature includes the named return variable cfg, but a different local variable compileConfig is declared and used to hold the configuration. This is inconsistent and can be confusing. It's better to use the named return variable cfg directly throughout the function.
func getJSLibCompileConfigByName(baseDir, jslibName, target string) (outputDir string, cfg compile.CompileConfig, err error) {
if jslibName == "" {
err = fmt.Errorf("jslib name cannot be empty")
return
}
var jslibDir string
var config compile.LibConfig
switch jslibName {
case "mquickjs":
config = mquickjs.GetMquickjsConfig()
jslibDir = filepath.Join(baseDir, config.String())
cfg = mquickjs.GetMquickjsCompileConfig(jslibDir, target)
default:
err = fmt.Errorf("unsupported jslib: %s", jslibName)
return
}
if needSkipDownload {
return jslibDir, cfg, err
}
if err = checkDownloadAndExtractLib(config.Url, jslibDir, config.ResourceSubDir); err != nil {
return
}
return jslibDir, cfg, nil
}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1517 +/- ##
==========================================
- Coverage 90.93% 89.62% -1.31%
==========================================
Files 44 46 +2
Lines 11528 12156 +628
==========================================
+ Hits 10483 10895 +412
- Misses 883 1081 +198
- Partials 162 180 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add GenerateHeaders() function to compile mqjs_stdlib tool for HOST and generate mquickjs_atom.h and mqjs_stdlib.h with -m32 flag - Call GenerateHeaders() in getJSLibCompileConfigByName() after download - Fix mergeConfig() to properly merge JSLib field from parent configs - Headers are cached and only regenerated if missing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add -fno-jump-tables which is required for ESP32 IRAM/Flash architecture. Without this flag, code in IRAM cannot access jump tables in Flash when Flash Cache is disabled, causing runtime crashes. Also add -std=gnu17 as recommended. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Generate mqjs_stdlib_llgo32.h header using ./mqjs_stdlib -m32 - Create stdlib_export.c to export js_stdlib symbol for Go linkname - Add __fpclassifyd implementation for embedded platforms - Add weak symbol stubs for platform-specific JS functions: js_print, js_gc, js_load, js_setTimeout, js_clearTimeout, js_date_now, js_performance_now - Update demo to use mquickjs v1.0.4 (32-bit JSValue support) - Add js_print override example showing how to implement print() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ation tests
Extended the mquickjs demo to showcase JavaScript function capabilities:
- Added runTest() helper function for cleaner test execution
- Added tests for defining JavaScript functions (add, factorial, multiply)
- Added tests for invoking previously defined functions
- Demonstrates that functions persist in the global scope across JSEval calls
This validates that mquickjs on ESP32 supports:
- Function declarations: function name() {}
- Function expressions: var name = function() {}
- Recursive functions (factorial)
- Cross-eval function persistence
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 participant
Summary
Changes
internal/crosscompile/compile/mquickjs/mquickjs.go- mquickjs download and compile configurationJSLibfield totargets.Configfor target configurationgetJSLibCompileConfigByName()function for jslib compilationUseTarget()Usage
Add
"jslib": "mquickjs"to target configuration JSON:{ "libc": "newlib-esp32", "rtlib": "compiler-rt", "jslib": "mquickjs" }Test plan
🤖 Generated with Claude Code