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

Abstracting out the terminal layer #3046

Open
SOF3 opened this issue Jul 19, 2019 · 7 comments
Open

Abstracting out the terminal layer #3046

SOF3 opened this issue Jul 19, 2019 · 7 comments
Labels
Category: Core Related to internal functionality Category: UI Related to the user interface (e.g. commands, terminal output) Type: Change Proposal RFCs (Request for Comments) on change ideas for PocketMine-MP

Comments

@SOF3
Copy link
Member

SOF3 commented Jul 19, 2019

Introduction

The orientation of logger

The Logger API is, as its name describes, designed for logging. Regular logging includes debug data, progress messages, event logs, abnormal behaviour warning and error messages. However, PocketMine and plugins have vastly abused the Logger as the terminal, gradually mixing interactive interface (such as command response messages) with logging. Moreover, it is impossible to log-without-print or print-without-log. Most processes use stdout as the logging output, but PocketMine outputs to both stdout and server.log, rendering stdout useless.

Interactive terminal interface

While PocketMine reads input from stdin and respond via stdout, terminal experience is unsatisfactory in general. To give a few examples of current limitations:

  • Logger only emits full-line output. There is no (obvious) way to write to terminal without possibility of race condition.
  • There is no reliable method of receiving input from the terminal (A stable way of reading a line from terminal #2554). In particular, if a plugin expects interactive response from the terminal (such as the /gc command confirmation), it either requires intercepting CommandEvent (which is undesirable since it is not a unique lock) or racing with CommandReader (which simply won't work half of the time), much less a thread-blocking read (not sure if that's even possible with the Snooze API).
  • Even if the plugin implements some stderr output that enables interactive terminal, since it cannot block stdout (without using hacks like ob_start etc.), the interactive terminal display would be very broken.

Not to mention that interactive terminal for a server software is... too ancient to say how ancient it is.

Possibility of GUI wrapping

The current distribution requires a semi-interactive terminal as the sole interface (although plugins can start an IPC socket server, some behaviour are not entirely reliable).

Proposal: CLI as a wrapper

The command line interface should only be one of the many possible methods of administration. However, the current code deeply assumes that terminal is the only way of interaction, and uses a lot of hacks to interact in this way.

The correct method is to remove the CLI entirely from the main parts of PocketMine. The main parts of PocketMine should be shipped as a library rather than as the binary itself. This implies that PocketMine-as-a-library cannot assume the existence of a console nor the PocketMine.php entry point. In fact, it should not even assume that there is a wrapper, because terminal in a 24/7 software is pointless -- you won't have a person watching the console 24/7.

Implications

Backward compatibility

This implies that console-related classes, in particular ConsoleCommandSender, would be removed from the core library. Although this might break some plugins, plugins should not have assumed console to be the sole super-user at all, so this is just correcting wrong behaviour. Plugins mostly use ConsoleCommandSender for permission checking, but this should be performed by permission checking. After all, the server administrator might not want to allow terminal to have full access, since the terminal may be directly exposed to other users (e.g. Multicraft interface).

Alert and Emergency

This proposal renders alert, emergency, etc. unreliable for notification. However, currently, these log levels do not beep the console anyway, so this does not change any fact -- alert does not imply user would be alerted. After all, most users won't be next to the console 24/7, so there is no such thing as "emergency". I would suggest that these log levels are misleading and should be removed, but this is another issue. Nevertheless, if a wrapper does support alerting the user, it can attach to the logger anyway.

@SOF3 SOF3 added Category: Core Related to internal functionality Category: UI Related to the user interface (e.g. commands, terminal output) labels Jul 19, 2019
@SOF3 SOF3 added this to the 5.0 milestone Jul 19, 2019
@lukeeey
Copy link
Contributor

lukeeey commented Jul 19, 2019

After all, the server administrator might not want to allow terminal to have full access, since the terminal may be directly exposed to other users (e.g. Multicraft interface).

The console already has all permissions?

@SOF3
Copy link
Member Author

SOF3 commented Jul 19, 2019

@lukeeey that's exactly what I'm saying. Someone who has read/write permission to stdin/stdout does not mean they have full access to the server.

@SOF3
Copy link
Member Author

SOF3 commented Dec 15, 2019

Edit: What I mentioned about log levels in the OP appears to be the result of some misconception. Do not rely on those, and any correction is welcome.

@dktapps
Copy link
Member

dktapps commented Dec 13, 2021

Related to #2543

@dktapps dktapps added the Type: Change Proposal RFCs (Request for Comments) on change ideas for PocketMine-MP label Dec 13, 2021
@dktapps dktapps removed this from the 5.0 milestone Jan 22, 2023
@SOF3
Copy link
Member Author

SOF3 commented Aug 13, 2023

Putting this afurther, I would propose that we completely remove terminal-related features in PocketMine-MP.phar. Instad, PocketMine just exposes something like an HTTP/WebSocket server. Running start.exe/./start (binary) would run php PocketMine-MP.phar as a subprocess, and connect to the server exposed in the subprocess and provide CLI normally. For detached deployment like Docker, the logs and command interface can be exposed either by exposing a container port or running docker exec -it pocketmine-server pocketminectl.

@dktapps
Copy link
Member

dktapps commented Aug 13, 2023

On a related note, a cool library I found: https://github.com/AmokHuginnsson/replxx

Implementing CLI as a wrapper would make it easier to use libraries like this, since we wouldn't need to bother about writing PHP interfaces for them.

@SOF3
Copy link
Member Author

SOF3 commented Aug 14, 2023

https://github.com/nushell/reedline also supports features like arrow keys and vi-based cursor motion etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core Related to internal functionality Category: UI Related to the user interface (e.g. commands, terminal output) Type: Change Proposal RFCs (Request for Comments) on change ideas for PocketMine-MP
Projects
None yet
Development

No branches or pull requests

3 participants