Skip to content
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

Detect proxy in desktop #396

Closed
wants to merge 17 commits into from
10 changes: 5 additions & 5 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.PHONY: all build client dist docs format install lint preview release server start test type version
.PHONY: all build client desktop dist docs format install lint release server start test type version


VERSION := $(shell node -p -e "require('./package.json').version")
Expand All @@ -15,6 +15,10 @@ build:
client:
npm run start

## Runs the Electron application with live reload (requires a running server).
desktop:
npm run desktop

## Runs electron-builder to package and build a ready for distribution app.
dist:
npm run dist
Expand All @@ -38,10 +42,6 @@ lint:
hatch run lint
npm run lint

## Runs the Electron application with live reload (requires a running server).
preview:
npm run preview

release:
git checkout main && git pull origin && git fetch -p
@git log --pretty=format:"%C(yellow)%h%Creset %s%Cgreen%d" --reverse -20
Expand Down
29 changes: 21 additions & 8 deletions desktop/python.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { join } from 'path'
import log from 'electron-log'
import toml from 'toml'
import * as system from './system'
import { getProxySettings } from 'get-proxy-settings'

export async function ensurePython() {
log.info('[ensurePython]', { path: settings.APP_PYTHON })
Expand All @@ -21,19 +22,18 @@ export async function ensurePython() {
export async function ensureLibraries() {
log.info('[ensureLibraries]')

const httpProxyUrl = await detectHttpProxyUrl()
const required = await readRequiredLibraries()
const installed = await readInstalledLibraries()
const missing = required.filter((spec) => !installed.includes(spec))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why would there be libraries missing from the installer?

Copy link
Collaborator Author

@roll roll Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses the concept of idempotency. The user at any point of time can have:

  • no installed dependencies
  • partially installed dependencies (if a new version of ODE adds a new dependency)
  • partially outdated dependencies (if a new version of ODE bumps a dependency's version)

So the installer calculates the diff and applies missing parts

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concept sounds elegant, but this means also that if a user downloads the app and then wants to open it for the first time and it's not connected to the internet, it will also throw the error, right? I think it is confusing why some additional libraries need to be installed if the user has already downloaded the binaries. I would switch to a 100% self contained version, this would also shorten the loading time at startup. @roll

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guergana I would also switch of course =)

if (!missing.length) return

await system.execFile(settings.PYTHON_TARGET, [
'-m',
'pip',
'install',
'--upgrade',
'--disable-pip-version-check',
...missing,
])
await system.execFile(
settings.PYTHON_TARGET,
['-m', 'pip', 'install', '--upgrade', '--disable-pip-version-check', ...missing],
// https://stackoverflow.com/a/41957788
{ env: { http_proxy: httpProxyUrl } }
)

log.info('[ensureLibraries]', { missing })
}
Expand Down Expand Up @@ -65,3 +65,16 @@ export async function readInstalledLibraries() {
log.info('[readInstalledLibraries]', { data })
return data
}

export async function detectHttpProxyUrl() {
log.info('[detectHttpProxyUrl]')

const proxy = await getProxySettings()
const url = proxy?.http ? proxy.http.toString() : undefined
const message = proxy?.http
? `proxy detected: http://***@${proxy.http.host}:${proxy.http.port}`
: `no proxy detected`

log.info('[detectHttpProxyUrl]', { message })
return url
}
26 changes: 19 additions & 7 deletions desktop/system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,29 @@ export async function findPort() {
return port
}

export async function execFile(path: string, args: string[], cwd?: string) {
log.info('[execFile]', { path, args, cwd })

const { stdout } = await execFilePromise(path, args, { cwd })
export async function execFile(
path: string,
args: string[],
opts?: { cwd?: string; env?: Record<string, string | undefined> }
) {
log.info('[execFile]', { path, args, opts })

const cwd = opts?.cwd
const env = { ...process.env, ...opts?.env }
const { stdout } = await execFilePromise(path, args, { cwd, env })
return stdout
}

export async function spawnFile(path: string, args: string[], cwd?: string) {
log.info('[spawnFile]', { path, args, cwd })
export async function spawnFile(
path: string,
args: string[],
opts?: { cwd?: string; env?: Record<string, string | undefined> }
) {
log.info('[spawnFile]', { path, args, opts })

const proc = cp.spawn(path, args, { cwd })
const cwd = opts?.cwd
const env = { ...process.env, ...opts?.env }
const proc = cp.spawn(path, args, { cwd, env })
proc.stdout.on('data', (data) => log.info(data.toString().trim()))
proc.stderr.on('data', (data) => log.error(data.toString().trim()))
proc.on('close', (code) => {
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@
"build": "electron-vite build",
"dist": "electron-builder",
"coverage": "sensible-browser coverage/index.html",
"desktop": "electron-vite dev",
"format": "prettier --write \"client/**/*.ts*\" && eslint --fix \"client/**/*.ts*\"",
"lint": "prettier --check \"client/**/*.ts*\" && eslint \"client/**/*.ts*\"",
"prepare": "husky install",
"preview": "electron-vite dev",
"start": "vite --open --port 8080",
"spec": "vitest run",
"test": "npm run lint && npm run type && npm run spec",
Expand All @@ -39,6 +39,7 @@
"dependencies": {
"@electron-toolkit/utils": "2.0.1",
"electron-log": "4.4.8",
"get-proxy-settings": "0.1.13",
"portfinder": "1.0.32",
"toml": "3.0.0"
},
Expand Down
8 changes: 5 additions & 3 deletions portal/content/docs/contributing/development.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,14 @@ npm run start

### Desktop

Previewing descript application:
> Note that you need to start the server as well

Running the desktop application:

```bash
make preview
make desktop
# OR
npm run preview
npm run desktop
```

## Documentation
Expand Down
Loading