-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: enable proxy support to resolve #1125
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
a259e56
fix: enable proxy support to resolve
bittoby 3e48ae6
Merge branch 'main' of https://github.com/bittoby/eigent into fix/dow…
bittoby 6bd6f43
fix: add connection cleanup for proxy tunnels and support separate HT…
bittoby 376dc0c
Merge branch 'main' of https://github.com/bittoby/eigent into fix/dow…
bittoby 777dbdc
feat: enhance proxy configuration with multi-source priority system a…
bittoby 209df00
enhance: reuse readEnvValue to make readEnvValueWithPriority
a7m-1st 3281eec
chore: update to local dev
a7m-1st d56c822
Merge branch 'main' into fix/download-proxy-support
a7m-1st File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import log from 'electron-log'; | |
| import fs from 'fs'; | ||
| import os from 'os'; | ||
| import path from 'path'; | ||
| import { maskProxyUrl, readEnvValueWithPriority } from './envUtil'; | ||
|
|
||
| export function getResourcePath() { | ||
| return path.join(app.getAppPath(), 'resources'); | ||
|
|
@@ -33,6 +34,69 @@ export function getBackendPath() { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get proxy environment variables with priority: | ||
| * 1. Process environment variables (inline/system) | ||
| * 2. .env.development file (development mode only) | ||
| * 3. Global ~/.eigent/.env config | ||
| * | ||
| * Returns an object with HTTP_PROXY, HTTPS_PROXY, NO_PROXY and lowercase variants | ||
| * if a proxy is configured, or an empty object if not. | ||
| * Supports separate HTTP and HTTPS proxy configurations. | ||
| */ | ||
| function getProxyEnvVars(): Record<string, string> { | ||
| // Check both uppercase and lowercase variants | ||
| const httpProxy = | ||
| readEnvValueWithPriority('HTTP_PROXY') || | ||
| readEnvValueWithPriority('http_proxy'); | ||
|
|
||
| const httpsProxy = | ||
| readEnvValueWithPriority('HTTPS_PROXY') || | ||
| readEnvValueWithPriority('https_proxy'); | ||
|
|
||
| // Return empty object if no proxy configured | ||
| if (!httpProxy && !httpsProxy) { | ||
| return {}; | ||
| } | ||
|
|
||
| // Log configured proxies | ||
| if (httpProxy) { | ||
| log.info( | ||
| `[INSTALL SCRIPT] HTTP Proxy configured: ${maskProxyUrl(httpProxy)}` | ||
| ); | ||
| } | ||
| if (httpsProxy) { | ||
| log.info( | ||
| `[INSTALL SCRIPT] HTTPS Proxy configured: ${maskProxyUrl(httpsProxy)}` | ||
| ); | ||
| } | ||
|
|
||
| // Get NO_PROXY configuration (with default for local connections) | ||
| const noProxy = readEnvValueWithPriority('NO_PROXY') || | ||
| readEnvValueWithPriority('no_proxy') || | ||
| 'localhost,127.0.0.1,.local'; | ||
|
|
||
| // Return all variants (some tools need uppercase, others lowercase) | ||
| // Filter out undefined values | ||
| const result: Record<string, string> = {}; | ||
|
|
||
| if (httpProxy) { | ||
| result.HTTP_PROXY = httpProxy; | ||
| result.http_proxy = httpProxy; | ||
| } | ||
|
|
||
| if (httpsProxy) { | ||
| result.HTTPS_PROXY = httpsProxy; | ||
| result.https_proxy = httpsProxy; | ||
| } | ||
|
|
||
| // Always set NO_PROXY when proxy is configured to avoid issues with local connections | ||
| result.NO_PROXY = noProxy; | ||
| result.no_proxy = noProxy; | ||
|
|
||
| return result; | ||
|
Comment on lines
+81
to
+97
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lastly I think we can simplify this logic drastically by returning: |
||
| } | ||
|
|
||
| export function runInstallScript(scriptPath: string): Promise<boolean> { | ||
| return new Promise<boolean>((resolve, reject) => { | ||
| const installScriptPath = path.join( | ||
|
|
@@ -42,8 +106,15 @@ export function runInstallScript(scriptPath: string): Promise<boolean> { | |
| ); | ||
| log.info(`Running script at: ${installScriptPath}`); | ||
|
|
||
| // Get proxy configuration from global .env file | ||
| const proxyEnv = getProxyEnvVars(); | ||
|
|
||
| const nodeProcess = spawn(process.execPath, [installScriptPath], { | ||
| env: { ...process.env, ELECTRON_RUN_AS_NODE: '1' }, | ||
| env: { | ||
| ...process.env, | ||
| ...proxyEnv, | ||
| ELECTRON_RUN_AS_NODE: '1', | ||
| }, | ||
| }); | ||
|
|
||
| let stderrOutput = ''; | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
I am not sure why this change was made. The original code had a complete ternary condition:
proxyUrl ? { ... } : {}When no proxy was provided, it returned an empty object and did not pollute the environment variables.
However, after this change, the ternary condition was removed and the return value of
readEnvValueWithPriority()is written directly into the object. When there is no proxy, all four keys arenull. Once passed to the child process, they become the string"null", whichuvthen treats as a proxy address during dependency installation, ultimately causing DNS resolution failures.@bittoby @a7m-1st
This PR was reverted because it caused DNS resolution errors during dependency installation.
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.
I updated the code yesterday, but I didn't manage to catch it while testing, perhaps bcz uv install didn't trigger in my case. Apologies for the hassle.
Thanks for reverting it @nitpicker55555
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.
thanks @nitpicker55555 for pointing this out, could @bittoby or @a7m-1st reopen a PR and add additional fix, then we can review & merge this
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.
@a7m-1st @Wendong-Fan I get one fix pr
In #1226, I tested and is work
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.
Hi there, thanks @nitpicker55555. In addition to the enhancements I added, I think your PR would do.