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
Closed
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
5 changes: 3 additions & 2 deletions desktop/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ app.whenReady().then(async () => {
log.info('# Start application')
electronApp.setAppUserModelId(settings.APP_USER_MODEL_ID)
const serverPort = await system.findPort()
const proxyUrls = await system.detectProxyUrls()

log.info('## Start client')
createBridge({ serverPort })
Expand All @@ -27,8 +28,8 @@ app.whenReady().then(async () => {
log.info('## Start server')
await resources.ensureRunner()
await python.ensurePython()
await python.ensureLibraries()
await server.runServer({ serverPort })
await python.ensureLibraries({ proxyUrls })
await server.runServer({ proxyUrls, serverPort })
}
})

Expand Down
29 changes: 20 additions & 9 deletions desktop/python.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import fs from 'fs'
import fsp from 'fs/promises'
import * as settings from './settings'
import * as types from './types'
import { join } from 'path'
import log from 'electron-log'
import toml from 'toml'
Expand All @@ -18,22 +19,19 @@ export async function ensurePython() {
log.info('[ensurePython]', { message })
}

export async function ensureLibraries() {
export async function ensureLibraries(props: { proxyUrls: types.IProxyUrls }) {
log.info('[ensureLibraries]')

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],
{ env: createEnvFromProxyUrls(props.proxyUrls) }
)

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

export function createEnvFromProxyUrls(proxyUrls: types.IProxyUrls) {
// https://stackoverflow.com/a/41957788
// https://stackoverflow.com/questions/8287628/proxies-with-python-requests-module
return {
// UNIX
HTTP_PROXY: proxyUrls.http,
HTTPS_PROXY: proxyUrls.https,
// Windows
http_proxy: proxyUrls.http,
https_proxy: proxyUrls.https,
}
}
13 changes: 11 additions & 2 deletions desktop/server.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,24 @@
import { spawnFile } from './system'
import { createEnvFromProxyUrls } from './python'
import * as settings from './settings'
import * as types from './types'
import log from 'electron-log'

export async function runServer({ serverPort }: { serverPort: number }) {
export async function runServer(props: {
proxyUrls: types.IProxyUrls
serverPort: number
}) {
const { serverPort } = props
log.info('[runServer]', { serverPort })

// Start server
const proc = spawnFile(
settings.PYTHON_TARGET,
['-m', 'server', settings.APP_TMP, '--port', serverPort.toString()],
process.resourcesPath
{
cwd: settings.DIST,
env: createEnvFromProxyUrls(props.proxyUrls),
}
)

return proc
Expand Down
45 changes: 39 additions & 6 deletions desktop/system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import log from 'electron-log'
import portfinder from 'portfinder'
import { is } from '@electron-toolkit/utils'
import * as settings from './settings'
import * as types from './types'
import { getProxySettings } from 'get-proxy-settings'
const execFilePromise = util.promisify(cp.execFile)

export async function findPort() {
Expand All @@ -17,17 +19,48 @@ export async function findPort() {
return port
}

export async function execFile(path: string, args: string[], cwd?: string) {
log.info('[execFile]', { path, args, cwd })
// It only works if both HTTP ans HTTPS proxies are set
// If only one of them is set, the `get-proxy-settings` library fails
// We cannot use `ses.resolveProxy(url)` because it does not return credentials
export async function detectProxyUrls() {
log.info('[detectProxyUrls]')
const proxyUrls: types.IProxyUrls = {}

const { stdout } = await execFilePromise(path, args, { cwd })
try {
const proxy = await getProxySettings()
if (proxy?.http) proxyUrls.http = proxy.http.toString()
if (proxy?.https) proxyUrls.https = proxy.https.toString()
} catch {}

log.info('[detectHttpProxyUrl]', { proxies: Object.keys(proxyUrls) })
return proxyUrls
}

export async function execFile(
path: string,
args: string[],
opts?: { cwd?: string; env?: Record<string, string | undefined> }
) {
const cwd = opts?.cwd
const thisEnv = opts?.env || {}
const fullEnv = { ...process.env, ...opts?.env }
log.info('[execFile]', { path, args, cwd, env: Object.keys(thisEnv) })

const { stdout } = await execFilePromise(path, args, { cwd, env: fullEnv })
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> }
) {
const cwd = opts?.cwd
const thisEnv = opts?.env || {}
const fullEnv = { ...process.env, ...opts?.env }
log.info('[spawnFile]', { path, args, cwd, env: Object.keys(thisEnv) })

const proc = cp.spawn(path, args, { cwd })
const proc = cp.spawn(path, args, { cwd, env: fullEnv })
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
1 change: 1 addition & 0 deletions desktop/types/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './proxy'
1 change: 1 addition & 0 deletions desktop/types/proxy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export type IProxyUrls = { http?: string; https?: string }
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
1 change: 1 addition & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"noUnusedParameters": true,
"noImplicitReturns": true,
"noFallthroughCasesInSwitch": false,
"allowImportingTsExtensions": true,
"experimentalDecorators": true,
"inlineSourceMap": true,
"inlineSources": true,
Expand Down