Conversation
|
@Pakchoioioi @Wendong-Fan could you help test the installers? https://github.com/eigent-ai/eigent/actions/runs/21521114829 if works well, we can close #868 |
|
|
||
| if (newContent !== content) { | ||
| fs.writeFileSync(filePath, newContent, 'utf-8'); | ||
| fs.chmodSync(filePath, 0o755); |
There was a problem hiding this comment.
GPT told me chmod may fail on windows but IDK. Not sure whether we need to only chmod for non windows only.
There was a problem hiding this comment.
Good catch! fs.chmodSync() works on Windows but has different behavior - it can only set/unset the read-only flag, while Unix permissions (0o755) are ignored. However, Node.js won't throw an error; it just silently skips the permission bits on Windows.
| // Use appropriate path separator for the platform | ||
| const pathSep = isWindowsPath ? '\\' : '/'; | ||
| const newHome = `{{PREBUILT_PYTHON_DIR}}${pathSep}${cpythonDir}${pathSep}${binDir}`; | ||
| content = content.replace(/^home\s*=\s*.+$/m, `home = ${newHome}`); |
There was a problem hiding this comment.
Yes, there should only have one "home" line.
| const cpythonDir = cpythonMatch[1]; | ||
|
|
||
| // Determine if this is Windows or Unix path | ||
| const isWindowsPath = originalHome.includes('\\') || originalHome.includes('Scripts'); |
There was a problem hiding this comment.
Should we prevent the case that for example
home = /opt/vendor/Scripts/cpython-3.10.19-linux-x86_64/bin
which is unix but it will detected as windows
Maybe something like
const isWindowsPath =
/^[A-Za-z]:\\/.test(originalHome) || originalHome.startsWith('\\\\') || originalHome.includes('\\');
There was a problem hiding this comment.
Your suggestion is more robust. I'll push the update and re-test the installers.
4pmtong
force-pushed
the
fix-win-0.0.82-add-mac-intel
branch
from
b1cc491 to
bd29b69
Compare
4pmtong
force-pushed
the
fix-win-0.0.82-add-mac-intel
branch
from
7c5cf83 to
bdda1b4
Compare
Wendong-Fan
left a comment
Wendong-Fan
left a comment
There was a problem hiding this comment.
LGTM overall! Just two small things.
| if ( | ||
| fs.existsSync(symlinkPath) || | ||
| fs.lstatSync(symlinkPath).isSymbolicLink() | ||
| ) { |
There was a problem hiding this comment.
When symlink doesnt exist, lstatSync throws, making the else block at L171 unreachable. Maybe wrap the lstat check in try/catch first?
| // Prebuild uses resources/prebuilt/venv; dev may use backend/.venv | ||
| const venvDir = fs.existsSync(prebuiltVenvDir) | ||
| ? prebuiltVenvDir | ||
| : path.join(backendDir, '.venv'); |
There was a problem hiding this comment.
If no venv exists, this fails with ENOENT. Worth adding a quick existence check with a friendly error?
3 participants
Description
This PR fixes Windows prebuilt venv path portability issues and adds macOS x64 / Intel CI build support.
Summary of changes:
Prebuilt venv portability and build pipeline
Windows path and placeholder fixes
CI: macOS x64 and Intel builds
build-view.yml.What is the purpose of this pull request?
fix #1084 #943 #998