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.

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

REPL: Load and write history to file #49

Closed
wants to merge 6 commits into from
Closed

REPL: Load and write history to file #49

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7cce09e
REPL: Load and write history to file
renatoalencar Jul 17, 2019
dc42914
REPL: Set a limit of entries for history
renatoalencar Jul 17, 2019
a9c2c1d
REPL: Fails first if find no home dir
renatoalencar Jul 17, 2019
6ede90e
REPL: Move and remove history variables
renatoalencar Jul 17, 2019
b0bfa82
REPL: Change historyFile instantiation
renatoalencar Jul 18, 2019
304d08d
REPL: Avoid creating empty file when history is empty
renatoalencar Jul 24, 2019
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
50 changes: 50 additions & 0 deletions tools/repl/repl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/PrettyStackTrace.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/ADT/SmallString.h"

#include "hermes/CompilerDriver/CompilerDriver.h"
#include "hermes/ConsoleHost/ConsoleHost.h"
Expand Down Expand Up @@ -42,6 +45,9 @@

#define C_STRING(x) #x

#define HISTORY_FILE ".hermes_history"
#define HISTORY_MAX_ENTRIES 500
mhorowitz marked this conversation as resolved.
Show resolved Hide resolved

using namespace hermes;

static llvm::cl::opt<std::string> PromptString(
Expand Down Expand Up @@ -222,6 +228,38 @@ static bool needsAnotherLine(llvm::StringRef input) {
return !stack.empty();
}

#if HAVE_LIBREADLINE
// Load history file or create it
std::error_code loadOrCreateHistoryFile(llvm::SmallString<128>& historyFile) {
llvm::Twine baseHistoryFile = llvm::Twine(HISTORY_FILE);
mhorowitz marked this conversation as resolved.
Show resolved Hide resolved

if (!llvm::sys::path::home_directory(historyFile)) {
// Use ENOENT here since it could not found a home directory
return std::error_code(ENOENT, std::system_category());
}

llvm::sys::path::append(historyFile, baseHistoryFile);

if (!llvm::sys::fs::exists(historyFile)) {
int fd;
auto err = llvm::sys::fs::openFileForWrite(llvm::Twine(historyFile), fd);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm running into some test errors around this code. This creates an empty file, which causes a confusing error from ::read_history():

$ rm ~/.hermes_history
$ ./bin/hermes-repl
Could not load history file: No such file or directory
>>

I did some experimentation, and this seems to be due to the file being empty, and not including the magic version header. If I start with cp /dev/null ~/.hermes_history I get the same warning.

And if I C-d immediately, so that a file is written with no contents, then when I run the next time, I get a different confusing error:

$ cat ~/.hermes_history
_HiStOrY_V2_
$ ./bin/hermes-repl
Could not load history file: Operation not permitted
>>

What's the purpose of creating the empty file here? Can we suppress the empty write, so we don't get a warning?

Finally, I noticed the history file is mode 600, even though my umask is 0022. Shouldn't it be mode 644? Although I see other history files with the same mode, so maybe this is a readline "feature", it's not universal: node's history file is mode 644.

Sorry for all this. The libreadline history support seems unfortunately brittle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes it could happens when you don't have a HOME environment variable set. Besides that, I really don't know what could happens.

The empty file is created in order to write_history to work, it opens the history file only on "append" mode, so if there is no file it returns a ENOENT error.

The LLVM file system API should actually create a 666 mode, but 600 is actually a good choice because of security issues on accessing history files.

I'm reading the libreadline source code in order to understand it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I'm avoiding creating empty files and ignoring ENOENT errors, because it happens in this case and that's an expected behavior.

About the permission, actually there is nothing we could do about it (at least using libreadline), both version 7 and 8 set the file permission hardcoded.

if (err) {
return err;
}

llvm::sys::fs::closeFile(fd);
}

auto err = ::read_history(historyFile.c_str());
if (err != 0) {
// Return a error_code object from a errno enum
return std::error_code(err, std::system_category());
}

return std::error_code();
}
#endif

// This is the vm driver.
int main(int argc, char **argv) {
llvm::sys::PrintStackTraceOnErrorSignal("Hermes REPL");
Expand Down Expand Up @@ -288,6 +326,14 @@ int main(int argc, char **argv) {

runtime->getHeap().runtimeWillExecute();

#if HAVE_LIBREADLINE
llvm::SmallString<128> historyFile = llvm::SmallString<128>();
mhorowitz marked this conversation as resolved.
Show resolved Hide resolved
auto historyErr = loadOrCreateHistoryFile(historyFile);
if (historyErr) {
llvm::errs() << "Could not load history file: " << historyErr.message() << '\n';
}
#endif

// SetUnbuffered because there is no explicit flush after prompt (>>).
// There is also no explicitly flush at end of line. (An automatic flush
// mechanism is not guaranteed to be present, from my experiment on Windows)
Expand All @@ -300,6 +346,10 @@ int main(int argc, char **argv) {
(readResult == ReadResult::INTERRUPT && code.empty())) {
// EOF or user exit on non-continuation line.
llvm::outs() << '\n';
#if HAVE_LIBREADLINE
::stifle_history(HISTORY_MAX_ENTRIES);
::write_history(historyFile.c_str());
mhorowitz marked this conversation as resolved.
Show resolved Hide resolved
#endif
return 0;
}

Expand Down