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

Fix repl crash when deno dir doesn't exist #2727

Merged
merged 8 commits into from
Aug 8, 2019

Conversation

crabmusket
Copy link
Contributor

Fixes #2719

@CLAassistant
Copy link

CLAassistant commented Aug 5, 2019

CLA assistant check
All committers have signed the CLA.

@@ -137,6 +143,18 @@ def test_lexical_scoped_variable(self):
self.assertEqual(err, '')
self.assertEqual(code, 0)

def test_missing_deno_dir(self):
current_deno_dir = os.environ["DENO_DIR"]
missing_deno_dir = os.path.join(current_deno_dir, 'missing_deno_dir')
Copy link
Member

Choose a reason for hiding this comment

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

This is just any path that doesn't exist, right? Rather than a subdirectory of the current DENO_DIR (which is a bit confusing) can you just do something like some_path_that_doesnt_exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the path does get created by the current (fixed) code when the repl exits, I wanted to make sure the path was somewhere that creating it wouldn't be a problem.

cli/repl.rs Outdated
match self.history_file.parent() {
Some(ref parent) => fs::create_dir_all(parent),
None => Ok(()),
}?;
Copy link
Member

@ry ry Aug 5, 2019

Choose a reason for hiding this comment

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

Rather than create the directory, maybe just print a warning and skip the save?

I think I'd prefer to keep all the directory creation in the deno_dir.rs module - this is repl.rs - so doesn't seem appropriate here.

@ry
Copy link
Member

ry commented Aug 5, 2019

Thanks for this fix BTW, it has been annoying me too.

@crabmusket
Copy link
Contributor Author

I think @bartlomieju suggested creating the dir if it doesn't exist when trying to save. How about moving the actual creation code into deno_dir and then calling it from repl?

@nayeemrmn
Copy link
Collaborator

DenoDir constructs DiskCache instances for deps and gen which get passed to the SourceFileFetcher and TsCompiler respectively. When caching, these invoke DiskCache::set() which creates the required parent directories on demand if they don't already exist.

Analogously: DenoDir should have a member history_file with type LogFile or something which gets passed to the Repl. When saving the history, it would invoke LogFile::append() which would behave similarly to DiskCache::set().

@ry
Copy link
Member

ry commented Aug 6, 2019

I think you can just print a warning and skip saving the history.

@crabmusket
Copy link
Contributor Author

Will do

@crabmusket
Copy link
Contributor Author

Not sure if it's my fault the builds are failing but it looks like networking issues? :s

@bartlomieju
Copy link
Member

@crabmusket try adding empty commit to reset CI

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 520bdb6 into denoland:master Aug 8, 2019
@crabmusket
Copy link
Contributor Author

Oh sh!t I was going to rebase some of those commits haha. Well, happy to have contributed! 🎉

@crabmusket crabmusket deleted the issue/2719-repl-crash branch August 8, 2019 11:33
@piscisaureus piscisaureus mentioned this pull request Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REPL crashes on exit if cache directory doesn't exist
5 participants