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

feat(app-shell-odd): Use sd-notify #12266

Merged
merged 6 commits into from
Mar 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/app-test-build-deploy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ jobs:
run: npm install -g npm@6
- name: check make version
run: make --version
- name: 'install libudev'
- name: 'install libudev and libsystemd'
if: startsWith(matrix.os, 'ubuntu')
run: sudo apt-get update && sudo apt-get install libudev-dev
- name: 'set complex environment variables'
Expand Down Expand Up @@ -205,7 +205,7 @@ jobs:
run: make --version
- name: 'install libudev'
if: startsWith(matrix.os, 'ubuntu')
run: sudo apt-get update && sudo apt-get install libudev-dev
run: sudo apt-get update && sudo apt-get install libudev-dev libsystemd-dev
- name: 'set complex environment variables'
id: 'set-vars'
uses: actions/[email protected]
Expand Down
3 changes: 3 additions & 0 deletions app-shell-odd/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,8 @@
"uuid": "3.2.1",
"winston": "3.1.0",
"yargs-parser": "13.1.2"
},
"optionalDependencies": {
"sd-notify": "2.8.0"
Comment on lines +63 to +65
Copy link
Member

Choose a reason for hiding this comment

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

shouldnt this be a regular dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't actually install correctly on not-linux - this is a thin wrapper around system libsystemd

}
}
8 changes: 8 additions & 0 deletions app-shell-odd/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ import { registerRobotLogs } from './robot-logs'
import { registerUpdate, updateLatestVersion } from './update'
import { registerRobotSystemUpdate } from './system-update'
import { getConfig, getStore, getOverrides, registerConfig } from './config'
import sdNotify from './systemd'

import type { BrowserWindow } from 'electron'
import type { Dispatch, Logger } from './types'

sdNotify.sendStatus('starting app')
const config = getConfig()
const log = createLogger('main')

Expand Down Expand Up @@ -42,6 +44,7 @@ app.once('window-all-closed', () => {

function startUp(): void {
log.info('Starting App')
sdNotify.sendStatus('loading app')
process.on('uncaughtException', error => log.error('Uncaught: ', { error }))
process.on('unhandledRejection', reason =>
log.error('Uncaught Promise rejection: ', { reason })
Expand Down Expand Up @@ -80,6 +83,11 @@ function startUp(): void {
})

log.silly('Global references', { mainWindow, rendererLogger })

ipcMain.once('dispatch', () => {
sdNotify.sendStatus('started')
Comment on lines +87 to +88
Copy link
Member

Choose a reason for hiding this comment

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

i think this should only send once since its in the startup fn and i assume this gets garbage collected and that event listener stops but we should make sure

Copy link
Member Author

Choose a reason for hiding this comment

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

since it uses once doesn't that mean it'll only be one time?

sdNotify.ready()
})
}

function createRendererLogger(): Logger {
Expand Down
36 changes: 36 additions & 0 deletions app-shell-odd/src/systemd.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { platform } from 'process'
// Provide systemd when possible and a default mocked instance, used only during
// dev workflows, when not.

interface SDNotify {
ready: () => void
sendStatus: (text: string) => void
}

const provideExports = (): SDNotify => {
try {
// This has to be a require because import is async when used functionally,
// and to catch import errors you have to use it functionally, so it can't be
// at top level, and we're doing this to put stuff in exports, so let's
// refactor this whenever we turn on top-level async
// eslint-disable-next-line @typescript-eslint/no-var-requires
const systemdNotify = require('sd-notify')
sfoster1 marked this conversation as resolved.
Show resolved Hide resolved
return { ready: systemdNotify.ready, sendStatus: systemdNotify.sendStatus }
} catch (err) {
if (platform === 'linux') {
console.error(
'Could not import systemd on linux, where it should be present. This is most likely because libsystemd bindings are not available, which hopefully means this is a dev setup.'
)
}
return {
ready: () => console.log('would send sd-notify ready'),
sendStatus: (text: string) =>
console.log(`would send sd-notify status ${text}`),
}
}
}

// Finally, we want this to work just like the actual sd-notify imports, so a default
// export it is
// eslint-disable-next-line import/no-default-export
export default provideExports()
12 changes: 12 additions & 0 deletions app-shell-odd/typings/sd-notify.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
declare module 'sd-notify' {
function ready(): void
function sendStatus(text: string): void
function startWatchdogMode(interval: number): void
function stopWatchdogMode(): void
export = {
ready,
sendStatus,
startWatchdogMode,
stopWatchdogMode,
}
}
9 changes: 8 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5434,7 +5434,7 @@ binary-extensions@^2.0.0:
resolved "https://registry.yarnpkg.com/binary-extensions/-/binary-extensions-2.2.0.tgz#75f502eeaf9ffde42fc98829645be4ea76bd9e2d"
integrity sha512-jDctJ/IVQbZoJykoeHbhXpOlNBqGNcwXJKJog42E5HDPUwQTSdjCHdihjj0DlnheQ7blbT6dHOafNAiS8ooQKA==

bindings@^1.5.0:
bindings@1.5.0, bindings@^1.5.0:
version "1.5.0"
resolved "https://registry.yarnpkg.com/bindings/-/bindings-1.5.0.tgz#10353c9e945334bc0511a6d90b38fbc7c9c504df"
integrity sha512-p2q/t/mhvuOj/UeLlV6566GD/guowlr0hHxClI0W9m7MWYkL1F0hLo+0Aexs9HSPCtR1SXQ0TD3MMKrXZajbiQ==
Expand Down Expand Up @@ -18737,6 +18737,13 @@ scrollbar-width@^3.1.1:
resolved "https://registry.yarnpkg.com/scrollbar-width/-/scrollbar-width-3.1.1.tgz#c62e63efa5934dac37b43da34f7550caca8444a2"
integrity sha1-xi5j76WTTaw3tD2jT3VQysqERKI=

[email protected]:
version "2.8.0"
resolved "https://registry.yarnpkg.com/sd-notify/-/sd-notify-2.8.0.tgz#a8e4477efac8426084e235d5d6a89f2af75571fd"
integrity sha512-e+D1v0Y6UzmqXcPlaTkHk1QMdqk36mF/jIYv5gwry/N2Tb8/UNnpfG6ktGLpeBOR6TCC5hPKgqA+0hTl9sm2tA==
dependencies:
bindings "1.5.0"

seek-bzip@^1.0.5:
version "1.0.6"
resolved "https://registry.yarnpkg.com/seek-bzip/-/seek-bzip-1.0.6.tgz#35c4171f55a680916b52a07859ecf3b5857f21c4"
Expand Down