use replxx instead of readline

[Willaaaaaaa]

What problem were solved in this pull request?

Issue Number: #558

Problem:
As I mentioned in the issue, students cannot use the arrow keys to browse the command history in the current cli. The existing code seems to want to use readline(GNU Readline library), which may conflict with the existing license. So I used replxx to improve this function in the cli.

What is changed and how it works?

Key changes:

• 3rd party dependency: replxx has been added as a git submodule
• CMakeLists.txt in observer
• related cpp source files

Basic logic:

1. The CliCommunicator class utilizes a replxx::Replxx instance to manage the interaction.
2. Every 5s, history saved to ./.miniob.history.

Other information

1. This PR focuses solely on the observer -P cli functionality. The obclient is NOT modified.
2. Default max_history_size = 1000.
3. Default config: when current_time - previous_history_save_time > 5, history_save.

[CLAassistant]

All committers have signed the CLA.

[nautaa]

please also change readline to replxx in src/client src/oblsm/client

[Willaaaaaaa]

please also change readline to replxx in src/client src/oblsm/client

I'll try to change tonight, thanks for advice!🌹

[nautaa]

i have another question, in my environment(centos 7), it seem will build failed. but for ubuntu 24.04, it's ok. do you have any idea?

/data/buduo.jyh/public_miniob/miniob/src/observer/net/cli_communicator.cpp:39: undefined reference to `replxx::Replxx::input(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'
/data/buduo.jyh/public_miniob/miniob/src/observer/net/cli_communicator.cpp:58: undefined reference to `replxx::Replxx::history_add(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'
../../lib/libobserver.a(cli_communicator.cpp.o): In function `read_command()':
/data/buduo.jyh/public_miniob/miniob/src/observer/net/cli_communicator.cpp:89: undefined reference to `replxx::Replxx::history_load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'
/data/buduo.jyh/public_miniob/miniob/src/observer/net/cli_communicator.cpp:99: undefined reference to `replxx::Replxx::history_save(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'
../../lib/libobserver.a(cli_communicator.cpp.o): In function `CliCommunicator::~CliCommunicator()':
/data/buduo.jyh/public_miniob/miniob/src/observer/net/cli_communicator.cpp:166: undefined reference to `replxx::Replxx::history_save(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'

[Willaaaaaaa]

@nautaa
Mine is Ubuntu 24.04, too.
I can only tell that this might have something to do with the linker.

I have googled the issue you mentioned. Some says it can be solved by adding pthread and dl to cmakelist as you have mentioned that we have done so. Some says it is relevant to the ABI mismatch issue, see here: https://gcc.gnu.org/onlinedocs/libstdc/manual/using_dual_abi.html.
I think you can try to "add -D_GLIBCXX_USE_CXX11_ABI=0 to your compiler options to make sure you're using the old ABI". If it really works, maybe we need to change the cmakelists.txt file to add the option.

[nautaa]

can we use a class LineReader to encapsulate, because it looks like there is too much duplicate code.

[nautaa]

@nautaa Mine is Ubuntu 24.04, too. I can only tell that this might have something to do with the linker.

I have googled the issue you mentioned. Some says it can be solved by adding pthread and dl to cmakelist as you have mentioned that we have done so. Some says it is relevant to the ABI mismatch issue, see here: https://gcc.gnu.org/onlinedocs/libstdc/manual/using_dual_abi.html. I think you can try to "add -D_GLIBCXX_USE_CXX11_ABI=0 to your compiler options to make sure you're using the old ABI". If it really works, maybe we need to change the cmakelists.txt file to add the option.

thanks for you advice! i will try it later

[Willaaaaaaa]

can we use a class LineReader to encapsulate, because it looks like there is too much duplicate code.

Yes! Exactly what I want to do last night. But I'm not sure where to put this class, so I just modified the original readline part to replxx.

Put the class in ./src/common/linereader, ./src/common/io or somewhere else? What do you think?

[nautaa]

can we use a class LineReader to encapsulate, because it looks like there is too much duplicate code.

Yes! Exactly what I want to do last night. But I'm not sure where to put this class, so I just modified the original readline part to replxx.

Put the class in ./src/common/linereader, ./src/common/io or somewhere else? What do you think?

./src/common/linereader is ok

[Willaaaaaaa]

@nautaa
OMG, I was just browsing the issues in replxx and suddenly realized that this project has not been maintained for a long time. Sadly, it seems that the author has passed away.
Will this cause any potential problems? There might be no one to maintain this repo.
Sorry, my bad. I haven't realized this. And I haven't encountered such situation before, I don't know how to handle this. Maybe I should look for an alternative? Maybe the current replxx is enough?

[nautaa]

@nautaa OMG, I was just browsing the issues in replxx and suddenly realized that this project has not been maintained for a long time. Sadly, it seems that the author has passed away. Will this cause any potential problems? There might be no one to maintain this repo. Sorry, my bad. I haven't realized this. And I haven't encountered such situation before, I don't know how to handle this. Maybe I should look for an alternative? Maybe the current replxx is enough?

maybe linenoise is better? looks like there are more projects use it.

Redis, MongoDB, Android and many other projects

If you want to introduce replxx continue, you can add a compile option. if someone has problems with replxx, he can turn off the compile option for replxx.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.21%. Comparing base (407af28) to head (79c2dab).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #559      +/-   ##
==========================================
+ Coverage   55.18%   55.21%   +0.02%     
==========================================
  Files         178      178              
  Lines       10338    10338              
==========================================
+ Hits         5705     5708       +3     
+ Misses       4633     4630       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Willaaaaaaa]

key changes

1. LineReaderManager class:
   The original code has duplicate code like my_readline() and is_exit_command(). Therefore, they are combined into the LineReaderManager class. This class contains a member variable LineReader, which determines to use either linenoise or replxx according to a CMake macro(see info 2).
2. LinenoiseReader class:
   A wrapper class for linenoise lib. Since the linenoise library has only two files, I placed them directly under ./src/common/linereader.

TODO

Since replxx and linenoise are C++ and C libs respectively, wrapping them both in a single class can cause issues such as memory leaks. In the latest version I submitted, linenoise works without problems, but replxx still leaks memory. Mainly because of some C-style string pointers. Really annoying! I'm fixing it currently, but I have some homework deadlines coming up soon, so the submission of the changes may not be fast.

infor

1. For replxx, I did not remove it mainly because:

   • This is a C++ lib based on linenoise which is a C lib. Therefore, it supports more functions than linenoise.
   • More existing key bindings(update: maybe not, i now have read the source code carefully. i find that it's only the key bindings aren't the same ones we use.)
   • It supports UTF-8 (but we don't seem to need this function)

2. Use replexx:

   mkdir build && cd build

   cmake .. -DUSEREPLXX=1 -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -DCMAKE_BUILD_TYPE=debug

   Use linenoise: cmake option default to not use replxx, so no seeting no replxx.

3. more info:

   • I think this one can work with replxx but have dup code and no linenoise support: commit 280fbbf96310377f2a5d398e05bd581cd725dfa6 (You reviewed this one.)
   • The latest one can work with linenoise but having memory leak with replxx.

[nautaa]

thanks for your effort, I will convert it to draft for now, you can mark it as ready to review when you're ready.

[Willaaaaaaa]

So, what I am about to do is:

1. Make linereader a submodule:
   Move linenoise from src/common/linereader, making it a git submodule.
   Added build and link configuration for this submodule in the top cmakelist (linenoise uses makefile, so I don't build it by bash build.sh). Then, updated the cmake configuration of each parts to include and link correctly.
2. Fix memory issues:
   As I said in previous comment, fixed the memory leak/access out-of-bounds....(I really hate this. I am not sure whether I can fix this.
3. Fix your feedbacks

Due to many courses having just concluded and exams approaching, I’ll be slower to fix bugs.
If urgent, maybe you can set a deadline for me so that I can make some arrangements.

Thank you for all your suggestions and comments.
As a second-year uni student, I don’t have much experience with large projects, so some mistakes are inevitable. I hope you guys can be understanding, and please feel free to challenge or correct me — it's a valuable opportunity for me to improve.🌹💕🫡

[nautaa]

So, what I am about to do is:

1. Make linereader a submodule:
   Move linenoise from src/common/linereader, making it a git submodule.
   Added build and link configuration for this submodule in the top cmakelist (linenoise uses makefile, so I don't build it by bash build.sh). Then, updated the cmake configuration of each parts to include and link correctly.
2. Fix memory issues:
   As I said in previous comment, fixed the memory leak/access out-of-bounds....(I really hate this. I am not sure whether I can fix this.
3. Fix your feedbacks

Due to many courses having just concluded and exams approaching, I’ll be slower to fix bugs. If urgent, maybe you can set a deadline for me so that I can make some arrangements.

Thank you for all your suggestions and comments. As a second-year uni student, I don’t have much experience with large projects, so some mistakes are inevitable. I hope you guys can be understanding, and please feel free to challenge or correct me — it's a valuable opportunity for me to improve.🌹💕🫡

thanks, There's no DDL here. just take your time! If you have any difficulties, feel free to get in touch.

[Willaaaaaaa]

Use replxx only. If anything goes wrong in the future, we can change to linenoise or replxx forked by clickhouse.
observer and obclient have 4 ways to quit: \q, exit, bye and interrupted. But oblsm_cli only have 1 way to exit: exit, because I followed the description in help and the macro only defines exit.
All the history files are saved to your currently wordingDir ./build and the name of the history file depends on which you use.

[nautaa]

LGTM

[Willaaaaaaa]

Thanks a lot!! It's my pleasure to contribute to miniob.
Really appreciate you guys' review and suggestions!! It's a valuable learning opportunity for me!🫡🥰🤩

[hnwyllmm]

Wonderful!
We have some more interesting issues. If you're interested in, please contact us. Please add my WeChat account if you want: hnwyllmm_126.