Make Linux UI logging behave like other platforms #12488
Conversation
|
r? @keybase/react-hackers cc @mmaxim |
|
Not sure how to test this end-to-end except cutting a new Linux release and then using that, but I've verified that opening the app dumps logs to the file. |
akalin-keybase
force-pushed
the
akalin/DESKTOP-7136
branch
from
5127dd6
to
0322505
Compare
| @@ -140,7 +140,7 @@ function logFileName(): string { | |||
| case 'darwin': | |||
| return `${logDir()}/${envedPathOSX[runMode]}.app.log` | |||
| case 'linux': | |||
| return '' // Linux is '' because we can redirect stdout | |||
There was a problem hiding this comment.
This has been around for a really long time, right? Did something change recently that caused the logs not to get written properly? Or was this stuff always broken on Linux?
There was a problem hiding this comment.
Yeap, been around for a while. Now that I think about it, I've seen the 'context deadline exceeded' error before when doing log send on Linux, so it seems to me that this was just always broken. 😱
There was a problem hiding this comment.
It shouldn't be broken, or at least not in the systemd case:
There was a problem hiding this comment.
Hmm, yeah looks like I misdiagnosed the original problem. The file does exist and is being upload -- the problem is that the UI log lines aren't being sent to stdout.
|
Discussed a bit with @chrisnojima but we should probably discuss on Monday when @oconnor663 gets back. |
|
@akalin-keybase should we do a hangout sometime today? |
|
Chatted with @oconnor663, we decided that I'd try removing the stdout logging and make |
akalin-keybase
force-pushed
the
akalin/DESKTOP-7136
branch
from
0322505
to
566127b
Compare
| @@ -97,7 +97,7 @@ func updaterBinName() (string, error) { | |||
|
|
|||
| // RunApp starts the app | |||
| func RunApp(context Context, log Log) error { | |||
| // TODO: Start app, see run_keybase: /opt/keybase/Keybase &>> "$logdir/Keybase.app.log" | |||
| // TODO: Start app, see run_keybase: /opt/keybase/Keybase | |||
There was a problem hiding this comment.
Not sure if this comment still applies but fixed it anyway
| @@ -4,8 +4,7 @@ Wants=keybase.service kbfs.service | |||
|
|
|||
| [Service] | |||
| Type=simple | |||
| ExecStartPre=/usr/bin/env mkdir -p %h/.cache/keybase | |||
| ExecStart=/usr/bin/env bash -c "/opt/keybase/Keybase &>> %h/.cache/keybase/Keybase.app.log" | |||
| ExecStart=/opt/keybase/Keybase | |||
There was a problem hiding this comment.
No need for the ExecStartPre since we create the log dir before dumping
There was a problem hiding this comment.
random thought...will this be affected by the electron bug that makes us redirect output to avoid a black screen?
There was a problem hiding this comment.
That sounds like a scary bug. Stdout is affecting what gets drawn on the screen?!
There was a problem hiding this comment.
It's super weird: electron/electron#12850
There was a problem hiding this comment.
Okay, tested that it still works.
| case 'win32': | ||
| return `${getenv('LOCALAPPDATA', '')}\\${envedPathWin32[runMode]}` | ||
| } | ||
| throw new Error(`Unknown platform ${process.platform}`) | ||
| } | ||
|
|
||
| function logFileName(): string { | ||
| // See DesktopLogFileName in go/libkb/constants.go. | ||
| // | ||
| // TODO: darwin and win32 cases are inconsistent with |
There was a problem hiding this comment.
darwin just happens to work for prod mode. not sure why win32 isn't bitten by the difference in case. @zanderz do you know?
| @@ -123,24 +123,31 @@ function findCacheRoot(): string { | |||
| } | |||
|
|
|||
| function logDir(): string { | |||
| // See LogDir() functions in go/libkb/home.go. | |||
| // | |||
| // TODO: darwin and win32 cases are inconsistent with their LogDir() | |||
There was a problem hiding this comment.
darwin just happens to work for prod mode, and so does win32 I'm guessing (do APPDATA and LOCALAPPDATA usually coincide?)
akalin-keybase
changed the title
|
Okay, this should be ready again for review @oconnor663 @jzila and @keybase/react-hackers |
| switch (process.platform) { | ||
| case 'darwin': | ||
| return `${getenv('HOME', '')}/Library/Logs` | ||
| case 'linux': | ||
| const linuxDefaultRoot = `${getenv('HOME', '')}/.local/share` | ||
| return `${getenv('XDG_DATA_HOME', linuxDefaultRoot)}/${envedPathLinux[runMode]}` | ||
| return findCacheRoot() |
There was a problem hiding this comment.
~/.local/share is the wrong place to put logs. Note that this also changes jsonDebugFileName
|
(Just tested on my machine: https://keybase.io/_/admin/logdump_viewer?603618386#67cf6afe93c10925c190321c ) |
|
bump (from @keybase/react-hackers) |
akalin-keybase
force-pushed
the
akalin/DESKTOP-7136
branch
from
566127b
to
58e2f63
Compare
This should cause UI logs to be uploaded on Linux (!).