refactor: Rust filesystem modules #4224

dubiousjim · 2020-03-02T18:45:41Z

This is part of a series of smaller PRs towards #4017.

It improves debug! calls, and does some other cleanup-type refactoring in Rust filesystem modules.

bartlomieju · 2020-03-02T20:29:14Z

cli/ops/fs.rs

@@ -6,7 +6,6 @@ use crate::op_error::OpError;
 use crate::ops::dispatch_json::JsonResult;
 use crate::state::State;
 use deno_core::*;
-use remove_dir_all::remove_dir_all;


I think this import can be removed (along with entry in Cargo.toml) and replaced with std::fs::remove_dir_all

Will do. Probably this is a good opportunity to start shifting over to the tokio::fs versions, as you propose in #4188.

Q: after removing the import from Cargo.toml, should I also commit an updated Cargo.lock? Or do you defer that until just before a release?

Yes, but #4188 should be done in a separate PR (I think I saw this code in one of your PRs)

Yes, Cargo.lock should be committed as well

Ok, there's a problem using std::fs::remove_dir_all as I do in 97c1a69 (below). Then the tests fail on Windows with a error: Uncaught Error: The directory is not empty. (os error 145). I assume this comes from removeAllSyncDirSuccess or removeAllDirSuccess, but those tests aren't currently written so as to catch that failure. So for the moment, I guess we should stick with remove_dir_all::remove_dir_all? It may be that the tokio::fs version works better on Windows.

Okay, makes sense 👍

cli/ops/fs.rs

bartlomieju · 2020-03-04T16:38:09Z

@dubiousjim heads up: I've made significant changes to test, could you resolve the conflicts?

ry

Hey @dubiousjim - I appreciate the effort but I'm having trouble understanding what's happening here. Is this fixing something? If so, it should add a test to demo what is fixed. If this patch is not fixing anything then it seems to just move code around without changing behavior - and it would be a net gain of code - which is undesirable.

ry · 2020-03-04T16:31:41Z

cli/fs.rs

@@ -95,7 +96,7 @@ pub fn make_temp(
 }

 pub fn mkdir(path: &Path, perm: u32, recursive: bool) -> std::io::Result<()> {
-  debug!("mkdir -p {}", path.display());
+  // debug!("mkdir -p {}", path.display());


Unnecessary. You can either remove the line or leave it as is, but leaving dead code isn't helpful. Suggest leaving it as is.

ry · 2020-03-04T16:33:16Z

cli/js/remove_test.ts

+  } catch (e) {
+    success = false;
+  }
+  assert(success);


What's the purpose of this try-catch if it's supposed to succeed?

Yeah, this PR has gotten confusing as it's evolved. It might be better for me to just close it and re-push a new version of it with some of these commits squashed together. But for explanation, here's what's going on/how we got here. Other than the debug! improvements, there's not much else in this PR except a bit of refactoring. One thing I did with the refactoring was move the third-party imports to cli/fs.rs, because I thought that was the guiding intent about how to divide work between cli/fs.rs and cli/ops/fs.rs. You explained in an earlier comment thread on this PR that no, things are probably just divided as they are because of the contingencies of past refactorings, and stuff should ideally all be in cli/ops/fs.rs if it can be. So I pushed revisions to this branch that moved things back. Also, @bartlomieju noticed that one of the imports I was moving back and forth between the files was of the third-party crate remove_dir_all. He said, we don't need that, there's such a function in std::fs. I hadn't noticed that, so I tried replacing the use of remove_dir_all::remove_dir_all with a plain std::fs::remove_dir_all. That turned out to work fine on Mac and Unix, but when I pushed it to the branch this PR is based on, the CI test for Windows turned out to fail. (See above comment exchange between me and @bartlomieju.) So I pushed another revision that re-instated the use of remove_dir_all::remove_dir_all, at least for the time being until we worked out a good solution for all platforms. Also, I noticed that the report of the failure on Windows was funny: it didn't say that a specific test failed, but there was just a error: Uncaught Error: The directory is not empty. (os error 145). I don't understand why. I tried adding the try ... catch blocks to those tests, so that if someone else breaks this in the same way later, they'd get a better error report. But maybe those fixes are stupid, and would fail in just the same way that the bare remove call did.

Anyway, hope that explains what's going on. Should I close this PR and resubmit a squashed/pruned version of it?

@ry, check out this branch on my fork. That has six commits on top of denoland/master, the first four being the good parts of this PR, and the last two replacing the use of remove_dir_all::remove_dir_all with std::fs::remove_dir_all. Based on what happened before, I'm expecting the branch to fail CI tests on Windows, and so the last two commits should be reverted/dropped. That would be the pruned version of this PR.

Confirmed that that fails, with only this information:

error: Uncaught Error: The directory is not empty. (os error 145)
$deno$/dispatch_json.ts:41:11
at unwrapResponse ($deno$/dispatch_json.ts:41:11)
at sendAsync ($deno$/dispatch_json.ts:96:10)
test tests::std_tests ... FAILED

failures:

---- tests::std_tests stdout ----
target_dir D:\a\deno\deno\target\release
##[error]thread 'tests::std_tests' panicked at 'assertion failed: status.success()', cli\tests\std_tests.rs:31:5
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace.
error: test failed, to rerun pass '-p deno --test std_tests'

failures:
tests::std_tests

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

##[error]Process completed with exit code 1.

Fails only on windows.

ry · 2020-03-04T16:33:27Z

cli/js/remove_test.ts

+    } catch (e) {
+      success = false;
+    }
+    assert(success);


dubiousjim · 2020-03-04T19:43:13Z

@dubiousjim heads up: I've made significant changes to test, could you resolve the conflicts?

Done.

Changed one use of std::fs to just fs, and uses of tokio::fs to tokio_fs.

dubiousjim · 2020-03-06T13:55:21Z

Am going to push --force a new branch to my fork overwriting this one, that contains just a pruned version of the useful parts of this PR, without the confusing history. Not sure how it will come out on this page. If it's a mess, I'll close this PR and open a new one.

dubiousjim · 2020-03-06T14:04:25Z

Hm, this page shows the push --force but doesn't present the commits for review. Guess I should close & reopen a different PR.

ry · 2020-03-06T14:11:53Z

@dubiousjim I think it's better to combine these changes (if at all) with some other ones. Closing.

dubiousjim mentioned this pull request Mar 2, 2020

Rename name/filename arguments to path #4227

Merged

bartlomieju reviewed Mar 2, 2020

View reviewed changes

ry requested changes Mar 4, 2020

View reviewed changes

dubiousjim added 4 commits March 6, 2020 08:51

Improved debug! calls in Rust filesystem modules

460e8dc

std::fs and tokio::fs

ec4ac65

Changed one use of std::fs to just fs, and uses of tokio::fs to tokio_fs.

refactor import of utime crate

50075a2

refactor use of std::os::unix::fs::symlink

59b9dfc

dubiousjim force-pushed the filesystem1b branch 2 times, most recently from 66ad22e to 59b9dfc Compare March 6, 2020 13:57

ry closed this Mar 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Rust filesystem modules #4224

refactor: Rust filesystem modules #4224

dubiousjim commented Mar 2, 2020

bartlomieju Mar 2, 2020

dubiousjim Mar 3, 2020

bartlomieju Mar 3, 2020

dubiousjim Mar 3, 2020

bartlomieju Mar 3, 2020

bartlomieju commented Mar 4, 2020

ry left a comment •

edited

Loading

ry Mar 4, 2020

ry Mar 4, 2020

dubiousjim Mar 4, 2020

dubiousjim Mar 4, 2020

dubiousjim Mar 4, 2020

dubiousjim Mar 4, 2020

ry Mar 4, 2020

dubiousjim commented Mar 4, 2020

dubiousjim commented Mar 6, 2020

dubiousjim commented Mar 6, 2020

ry commented Mar 6, 2020

refactor: Rust filesystem modules #4224

refactor: Rust filesystem modules #4224

Conversation

dubiousjim commented Mar 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Mar 4, 2020

ry left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubiousjim commented Mar 4, 2020

dubiousjim commented Mar 6, 2020

dubiousjim commented Mar 6, 2020

ry commented Mar 6, 2020

ry left a comment •

edited

Loading