Feat fix SITL #2492
Feat fix SITL #2492
Conversation
|
You are nearing your monthly Qodo Merge usage quota. For more information, please visit here. PR Compliance Guide 🔍All compliance sections have been disabled in the configurations. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| function getSitlBasePath() { | ||
| if (app.isPackaged) { | ||
| return path.join(process.resourcesPath, 'sitl'); | ||
| } else { | ||
| return path.join(process.cwd(), 'resources', 'public', 'sitl'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: In the getSitlBasePath function, replace process.cwd() with a relative path from __dirname to make resource location in development mode more reliable. [possible issue, importance: 7]
| function getSitlBasePath() { | |
| if (app.isPackaged) { | |
| return path.join(process.resourcesPath, 'sitl'); | |
| } else { | |
| return path.join(process.cwd(), 'resources', 'public', 'sitl'); | |
| } | |
| } | |
| function getSitlBasePath() { | |
| if (app.isPackaged) { | |
| return path.join(process.resourcesPath, 'sitl'); | |
| } else { | |
| return path.join(__dirname, '..', '..', 'resources', 'public', 'sitl'); | |
| } | |
| } |
There was a problem hiding this comment.
I don't think that ".." is better than (process.cwd()
There was a problem hiding this comment.
Question:
I don't think that .. is better than (process.cwd()
Answer:
You can reduce (or prevent) this kind of suggestion by adding a repo-level instruction that Qodo Merge should not “automatically” prefer __dirname-based paths over process.cwd() in Electron/dev-mode, unless it can prove a concrete failure mode in your repo setup.
Add to your .pr_agent.toml (or wiki .pr_agent.toml) something like:
[pr_code_suggestions]
extra_instructions = """\
When reviewing Electron/Node path handling:
- Do NOT suggest replacing process.cwd() with __dirname-based relative paths by default.
- Only propose changing the base path if you can explain a concrete reliability issue in this repository's workflow (e.g., cwd differs between dev scripts, tests, packaging, or CI).
- If unsure, ask for context (how the app is launched in dev) instead of proposing a path refactor.
"""If you still want the tool to mention the tradeoff but avoid a prescriptive patch, you can add:
[pr_code_suggestions]
extra_instructions = """\
For suggestions about path base directories, prefer explaining pros/cons (cwd vs __dirname) and avoid giving a code diff unless the current approach is clearly broken in this repo.
"""Relevant Sources:
| ipcMain.on('startChildProcess', (_event, command, args, opts) => { | ||
| child_process.start(path.join(__dirname, 'sitl', command), args, opts, mainWindow); | ||
| child_process.start(path.join(getSitlBasePath(), command), args, opts, mainWindow); | ||
| }); |
There was a problem hiding this comment.
Suggestion: In the startChildProcess handler, conditionally append the .exe extension to the command on Windows to ensure the executable can be found. [possible issue, importance: 8]
| ipcMain.on('startChildProcess', (_event, command, args, opts) => { | |
| child_process.start(path.join(__dirname, 'sitl', command), args, opts, mainWindow); | |
| child_process.start(path.join(getSitlBasePath(), command), args, opts, mainWindow); | |
| }); | |
| ipcMain.on('startChildProcess', (_event, command, args, opts) => { | |
| const cmdName = process.platform === 'win32' ? `${command}.exe` : command; | |
| const cmdPath = path.join(getSitlBasePath(), cmdName); | |
| child_process.start(cmdPath, args, opts, mainWindow); | |
| }); |
There was a problem hiding this comment.
command already contains right executable name
There was a problem hiding this comment.
Question:
coomand already contains right executable name
Answer:
Add an improve-tool instruction to avoid platform-specific “append .exe” suggestions when the executable name is already provided by the caller.
[pr_code_suggestions]
extra_instructions = """\
- When reviewing process-spawn/child-process code, assume the `command` argument already contains the correct executable name (including any needed extension like `.exe`).
- Do NOT suggest appending `.exe` or altering executable naming unless the PR explicitly shows a bug where an extension is missing.
"""If you want this only for a specific PR run (instead of a persistent config), you can also run:
/improve --pr_code_suggestions.extra_instructions="..."
Relevant Sources:
| ipcMain.on('startChildProcess', (_event, command, args, opts) => { | ||
| child_process.start(path.join(__dirname, 'sitl', command), args, opts, mainWindow); | ||
| child_process.start(path.join(getSitlBasePath(), command), args, opts, mainWindow); | ||
| }); |
There was a problem hiding this comment.
Suggestion: Validate/normalize command from IPC (reject .., absolute paths, and unknown executables) and ensure the resolved path stays within the SITL base directory before starting the child process. [Learned best practice, importance: 6]
| ipcMain.on('startChildProcess', (_event, command, args, opts) => { | |
| child_process.start(path.join(__dirname, 'sitl', command), args, opts, mainWindow); | |
| child_process.start(path.join(getSitlBasePath(), command), args, opts, mainWindow); | |
| }); | |
| ipcMain.on('startChildProcess', (_event, command, args, opts) => { | |
| const base = getSitlBasePath(); | |
| const candidate = path.resolve(base, command); | |
| if (!candidate.startsWith(path.resolve(base) + path.sep) || path.isAbsolute(command) || command.includes('..')) { | |
| throw new Error(`Invalid SITL command path: ${command}`); | |
| } | |
| child_process.start(candidate, args, opts, mainWindow); | |
| }); |
There was a problem hiding this comment.
probably we don't need check if sitl exe file exists
There was a problem hiding this comment.
Question:
probably we don't need check if sitl exe file exists
Answer:
You can steer /improve away from “check if the SITL executable exists” recommendations, while still keeping the important path-traversal / allowlist validation guidance, by adding a repo-level instruction like:
[pr_code_suggestions]
extra_instructions = """\
- Do NOT suggest checking whether an executable/file exists on disk (e.g., fs.existsSync, access checks) unless the PR explicitly adds error handling for missing files.
- For IPC-launched processes, prioritize security validation: reject absolute paths, '..' traversal, and enforce that the resolved path stays within an approved base directory; prefer allowlisting known commands when possible.
"""Relevant Sources:
error414
force-pushed
the
feat-fix-sitl
branch
2 times, most recently
from
e3dcf5a to
8eb245a
Compare
turn off strip for arm64 executable file in x86 linux build
error414
force-pushed
the
feat-fix-sitl
branch
from
8eb245a to
2964acd
Compare
|
Should no longer be necessary once #2438 has been merged. |
|
Thanks! I did this with a few tweaks as #2496 |
3 participants
User description
Note: I don't think this PR should be merget to updatream, my solution is quite brutal force, but it shows where is the issue. My knowledge is not enought to fix vite build correctly.
There were two bugs.
exe files were not copied to package, should solve bfb2f99#diff-0afe94b5ad1971b035d2d538a77509ebe4f7ecd7a16313e3ac63025dfd865f8bR12
wrong patches to executable files for dev and for package, should solve bfb2f99#diff-34ac9400ad8aa4c5573db2b1e27555eb25fce827f5ed8a86907ff1a492f76636R214
Tested, it workd for
npm run startand for
npm run packagePR Type
Bug fix
Description
This description is generated by an AI tool. It may have inaccuracies
Add SITL resources to packaged application via extraResource configuration
Implement dynamic SITL base path resolution for dev and production environments
Fix executable file path resolution to work in both development and packaged modes
Diagram Walkthrough
File Walkthrough
forge.config.js
Add SITL resources to package configurationforge.config.js
extraResourceconfiguration to include SITL resources in packagedapplication
resources/public/sitldirectory to be copied during packagingmain.js
Implement dynamic SITL path resolution for environmentsjs/main/main.js
getSitlBasePath()function to resolve SITL directory pathdynamically
app.isPackagedto determine correct path: packaged vsdevelopment
startChildProcesshandler to use dynamic path resolutioninstead of hardcoded
__dirname