Skip to content

feat: instance "name", test ids, store pipes in stdpath(state) #8519

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.

Sign up for GitHub

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

Jump to bottom
Merged
justinmk merged 3 commits into from
Jun 16, 2022
Merged

feat: instance "name", test ids, store pipes in stdpath(state) #8519

justinmk merged 3 commits into from
Jun 16, 2022

Conversation

@justinmk
Copy link
Member

@justinmk justinmk commented Jun 10, 2018 •
edited
Loading

Problem:

  • Unix sockets are created in random /tmp dirs.
    • /tmp is messy, unclear when OSes actually clear it.
    • The generated paths are very ugly. This adds friction to reasoning about which paths belong to which Nvim instances.
  • No way to provide a human-friendly way to identify Nvim instances in logs or server addresses.
  • No way to correlate tests with log messages.

Solution:

  • Store unix sockets in stdpath('state'). Closes Put socket into $XDG_RUNTIME_DIR #3517
  • Allow --listen "name" and serverstart("name") to given a name (which is appended to a generated path).
  • Generate a test-id for each test and set that as the "name" of Nvim instances spawned from that test.

Details

Each test gets a test id which looks like "T123". This also appears in the log file. Child processes spawned from a test appear in the logs with the parent name followed by "/c". Example:

DBG 2022-06-15T18:37:45.226 T57.58016.0   UI: flush
DBG 2022-06-15T18:37:45.226 T57.58016.0   inbuf_poll:442: blocking... events_enabled=0 events_pending=0
DBG 2022-06-15T18:37:45.227 T57.58016.0/c UI: stop
INF 2022-06-15T18:37:45.227 T57.58016.0/c os_exit:595: Nvim exit: 0
DBG 2022-06-15T18:37:45.229 T57.58016.0   read_cb:118: closing Stream (0x7fd5d700ea18): EOF (end of file)
INF 2022-06-15T18:37:45.229 T57.58016.0   on_process_exit:400: exited: pid=58017 status=0 stoptime=0

TODO:

  • is stdpath(state) the right place to store Nvim sockets? Put socket into $XDG_RUNTIME_DIR #3517 implies not. I'm hoping someone will raise the alarm if there is a security issue with storing sockets in ~/.local/state/.

@marvim marvim added the WIP label Jun 10, 2018
@justinmk justinmk force-pushed the busted-pid branch 2 times, most recently from f8ddde8 to 74419b8 Compare May 26, 2022 12:33
@justinmk justinmk changed the title [WIP] test/busted: log PID next to test title test/busted: log PID next to test title May 26, 2022
@justinmk justinmk changed the title test/busted: log PID next to test title test/busted: log test id next to test title May 26, 2022
kylo252
kylo252 reviewed May 26, 2022
View reviewed changes
@justinmk justinmk force-pushed the busted-pid branch 3 times, most recently from 04b6e20 to b08237a Compare May 29, 2022 21:25
@justinmk justinmk changed the title test/busted: log test id next to test title feat(test): log test id next to test title May 29, 2022
@justinmk justinmk force-pushed the busted-pid branch 4 times, most recently from 25b61cb to 981e9a8 Compare June 1, 2022 18:28
@justinmk justinmk mentioned this pull request Jun 1, 2022
Merged
@justinmk justinmk force-pushed the busted-pid branch 2 times, most recently from 4e4362e to ac4a5f8 Compare June 3, 2022 12:05
@justinmk justinmk changed the title feat(test): log test id next to test title feat: instance "name", test ids Jun 16, 2022
@justinmk justinmk changed the title feat: instance "name", test ids feat: instance "name", test ids, store pipes in stdpath(state) Jun 16, 2022
@justinmk justinmk removed the WIP label Jun 16, 2022
@justinmk justinmk force-pushed the busted-pid branch 2 times, most recently from e4acffe to 62e3211 Compare June 16, 2022 02:11
justinmk added 3 commits June 15, 2022 19:23
@justinmk
feat(logging): include test-id in log messages
8f06520 
Problem:
1. Log messages (especially in CI) are hard to correlate with tests.
2. Since b353a5c neovim#11886, dumplog() prints the logs next to test
   failures. This is noisy and gets in the way of the test results.

Solution:
1. Associate an incrementing id with each test and include it in log
   messages.
    - FUTURE: add v:name so Nvim instances can be formally "named"?
2. Mention "child" in log messages if the current Nvim is a child (based
   on the presence of $NVIM).

BEFORE:

    DBG … 12345      UI: event
    DBG … 12345      log_server_msg:722: RPC ->ch 1: …
    DBG … 12345      UI: flush
    DBG … 12345      inbuf_poll:444: blocking... events_enabled=1 events_pending=0
    DBG … 23454      UI: stop
    INF … 23454      os_exit:594: Nvim exit: 0

AFTER:

    DBG … T57        UI: event
    DBG … T57        log_server_msg:722: RPC ->ch 1: …
    DBG … T57        UI: flush
    DBG … T57        inbuf_poll:444: blocking... events_enabled=1 events_pending=0
    DBG … T57/child  UI: stop
    INF … T57/child  os_exit:594: Nvim exit: 0
@justinmk
test(report): formatting, drop dumplog()
b6467df 
Don't need to dumplog() on each failed test because we now have test-ids
that associate log messages with tests.
@justinmk
feat(server): instance "name", store pipes in stdpath(state)
1f2c2a3 
Problem:
- Unix sockets are created in random /tmp dirs.
  - /tmp is messy, unclear when OSes actually clear it.
  - The generated paths are very ugly. This adds friction to reasoning
    about which paths belong to which Nvim instances.
- No way to provide a human-friendly way to identify Nvim instances in
  logs or server addresses.

Solution:
- Store unix sockets in stdpath('state')
- Allow --listen "name" and serverstart("name") to given a name (which
  is appended to a generated path).

TODO:
- is stdpath(state) the right place?
kylo252
kylo252 reviewed Jun 16, 2022
View reviewed changes
test/busted/outputHandlers/nvim.lua
Comment on lines +197 to +199
if failureCount > 0 or errorCount > 0 then
io.write(global_helpers.read_nvim_log(nil, true))
end
Copy link
Contributor

@kylo252 kylo252 Jun 16, 2022

Choose a reason for hiding this comment

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

isn't it better to have this in either handler.testEnd() or handler.error()?

otherwise, I don't see much use of dumping the tail of the log. however, it would work if read_nvim_log() was grepping for test_id

Copy link
Member Author

@justinmk justinmk Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  1. We've tried testEnd() (that's the current state of master). It is too noisy. This PR intentionally avoids that.
  2. The tail should be short because most tests don't log (CI sets loglevel=WARN)
  3. The logs in theory may be useful because of the test ids. The next step is to use this on master for awhile and see how useful it is or isn't.

if read_nvim_log() was grepping for test_id

That could be interesting. At this point the idea is just to see if it's useful at all, and then improve things if and only if need becomes clear.

@justinmk
Copy link
Member Author

justinmk commented Jun 16, 2022 •
edited
Loading

Merging this but will continue iterating. Especially interested in strong arguments against storing sockets / named pipes in ~/.local/state/.

https://wiki.archlinux.org/title/XDG_Base_Directory says:

XDG_RUNTIME_DIR
Used for non-essential, user-specific data files such as sockets, named pipes, etc.
May be subject to periodic cleanup. [unless modified within last 6 hours]

This would be a complication, but if we touch it periodically the socket won't be removed. #17093 (comment)

Must be on the local filesystem.

That might be an advantage of XDG_RUNTIME_DIR, if it implies that XDG_STATE_DIR is potentially non-local.

@justinmk justinmk merged commit c57f6b2 into neovim:master Jun 16, 2022
@justinmk justinmk deleted the busted-pid branch June 16, 2022 23:23
@justinmk justinmk mentioned this pull request Jun 17, 2022
Closed
@kylo252
Copy link
Contributor

kylo252 commented Jun 18, 2022

That might be an advantage of XDG_RUNTIME_DIR, if it implies that XDG_STATE_DIR is potentially non-local.

For the record, $XDG_RUNTIME_DIR is managed by pam_systemd, so any (Linux) platform that doesn't use systemd and/or logind will have a problem with this.

@justinmk justinmk mentioned this pull request Jun 19, 2022
Closed
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 20, 2022
@justinmk
feat: stdpath('runtime'), create tempdir at "/tmp/nvim.user/"
f4518ef 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Introduce stdpath('runtime'), and store sockets there.
- Establish "/tmp/nvim.user/" as the root of all Nvim temp dirs.

closes neovim#3517
closes neovim#17093
@Rahix
Copy link

Rahix commented Jun 26, 2022

Especially interested in strong arguments against storing sockets / named pipes in ~/.local/state/

I would not put runtime files like sockets & named pipes into anything beneach $HOME for two reasons:

  1. Because it is part of the home directory, it is potentially non-local. Some people have a setup where the home directory is mounted via NFS on multiple hosts and each should probably have separate nvim runtime files.
  2. Because it is a persistent location. There is no value in storing these kind of files on a filesystem backed by a real disk. A tmpfs location is much cleaner here, I would say.

From my understanding $XDG_STATE_HOME is meant for persistent state instead of runtime files. Similar to how a database would store its data in /var/lib/, this is the per-user equivalent. The same database would then maybe have a connection socket in /run/ - and for this the per-user equivalent is $XDG_RUNTIME_DIR.

For the record, $XDG_RUNTIME_DIR is managed by pam_systemd, so any (Linux) platform that doesn't use systemd and/or logind will have a problem with this.

The standard explicitly suggests to implement a fallback to /tmp or an equivalent when $XDG_RUNTIME_DIR is not available. I think this should cover any non-systemd platforms.

@justinmk
Copy link
Member Author

justinmk commented Jun 26, 2022

@Rahix Thank you. In #18993 I am working on XDG_RUNTIME_DIR support and will use that for storage of sockets, and this will be under /tmp by default.

@justinmk justinmk mentioned this pull request Jun 26, 2022
Open
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 29, 2022
@justinmk
feat: stdpath('runtime'), create tempdir at "/tmp/nvim.user/"
399419c 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Introduce stdpath('runtime'), and store sockets there.
- Establish "/tmp/nvim.user/" as the root of all Nvim temp dirs.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 29, 2022
@justinmk
feat: stdpath('runtime'), create tempdir at "/tmp/nvim.user/"
216f265 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Introduce stdpath('runtime'), and store sockets there.
- Establish "/tmp/nvim.user/" as the root of all Nvim temp dirs.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 29, 2022
@justinmk
feat: stdpath('runtime'), create tempdir at "/tmp/nvim.user/"
e0199ff 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Introduce stdpath('runtime'), and store sockets there.
- Establish "/tmp/nvim.user/" as the root of all Nvim temp dirs.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 29, 2022
@justinmk
feat: stdpath('runtime'), create tempdir at "/tmp/nvim.user/"
06ace16 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Introduce stdpath('runtime'), and store sockets there.
- Establish "/tmp/nvim.user/" as the root of all Nvim temp dirs.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 29, 2022
@justinmk
feat: stdpath('runtime'), create tempdir at "/tmp/nvim.user/"
0cf026b 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Introduce stdpath('runtime'), and store sockets there.
- Establish "/tmp/nvim.user/" as the root of all Nvim temp dirs.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 29, 2022
@justinmk
feat: stdpath('runtime'), create tempdir at "/tmp/nvim.user/"
72a2217 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Introduce stdpath('runtime'), and store sockets there.
- Establish "/tmp/nvim.user/" as the root of all Nvim temp dirs.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 30, 2022
@justinmk
feat: stdpath('runtime'), create tempdir at "/tmp/nvim.user/"
8184aa0 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Introduce stdpath('runtime'), and store sockets there.
- Establish "/tmp/nvim.user/" as the root of all Nvim temp dirs.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 30, 2022
@justinmk
feat: stdpath('run'), create tempdir at "/tmp/nvim.user/"
5b5a052 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Introduce stdpath('run') ($XDG_RUNTIME_DIR), store sockets there.
- Establish "/tmp/nvim.user/" as the tempdir root shared by all Nvims.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 30, 2022
@justinmk
feat: stdpath('run'), create tempdir at "/tmp/nvim.user/"
042e49e 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Introduce stdpath('run') ($XDG_RUNTIME_DIR), store sockets there.
- Establish "/tmp/nvim.user/" as the tempdir root shared by all Nvims.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
justinmk added a commit that referenced this pull request Jun 30, 2022
@justinmk
feat: stdpath('run'), /tmp/nvim.user/ #18993
f50135a 
Problem:
- Since c57f6b2 #8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Store sockets in stdpath('run') ($XDG_RUNTIME_DIR).
- Establish "/tmp/nvim.user/" as the tempdir root shared by all Nvims.
- Make ok() actually useful.
- Introduce assert_nolog().

closes #3517
closes #17093
kraftwerk28 pushed a commit to kraftwerk28/neovim that referenced this pull request Jul 6, 2022
@justinmk @kraftwerk28
feat: stdpath('run'), /tmp/nvim.user/ neovim#18993
c141189 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Store sockets in stdpath('run') ($XDG_RUNTIME_DIR).
- Establish "/tmp/nvim.user/" as the tempdir root shared by all Nvims.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
oxalica added a commit to oxalica/neovim-remote that referenced this pull request Oct 2, 2022
@oxalica
Fix tests for neovim 0.8.0
0b8572f 
Since neovim/neovim#8519, `NVIM_LISTEN_ADDRESS` is only directly used
when it contains colons or (back-)slashes. Otherwise it is concatanated
with PID suffix as tempdir prefix.
oxalica added a commit to oxalica/neovim-remote that referenced this pull request Oct 2, 2022
@oxalica
Fix tests for neovim 0.8.0
56d2a40 
Since neovim/neovim#8519, `NVIM_LISTEN_ADDRESS` is only directly used
when it contains colons or (back-)slashes. Otherwise it is concatanated
with a tempdir prefix and suffixes to form the final path.
@oxalica oxalica mentioned this pull request Oct 2, 2022
Open
smjonas pushed a commit to smjonas/neovim that referenced this pull request Dec 31, 2022
@justinmk @smjonas
feat: stdpath('run'), /tmp/nvim.user/ neovim#18993
f54d0a4 
Problem:
- Since c57f6b2 neovim#8519, sockets are created in ~/.local/… but XDG
  spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which
  implies that XDG_STATE_DIR is potentially non-local.
- Not easy to inspect Nvim-created temp files (for debugging etc).

Solution:
- Store sockets in stdpath('run') ($XDG_RUNTIME_DIR).
- Establish "/tmp/nvim.user/" as the tempdir root shared by all Nvims.
- Make ok() actually useful.
- Introduce assert_nolog().

closes neovim#3517
closes neovim#17093
@vanaigr vanaigr mentioned this pull request Feb 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@kylo252 kylo252 kylo252 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Put socket into $XDG_RUNTIME_DIR

4 participants

@justinmk @kylo252 @Rahix @marvim