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

rustlings watch fails silently #472

Closed
jounathaen opened this issue Jul 18, 2020 · 8 comments · Fixed by #582
Closed

rustlings watch fails silently #472

jounathaen opened this issue Jul 18, 2020 · 8 comments · Fixed by #582
Labels
A-source Area: CLI source C-bug Category: Bug

Comments

@jounathaen
Copy link

Hi,
I can't get rustlings to work correctly. If I run rustlings watch, the program fails silently.
Apparently the problem is, that

watcher.watch(Path::new("./exercises"), RecursiveMode::Recursive)?;
fails on my system (Fedora 32, nothing special AFAIK) and this error only handled with an is_ok() in
if matches.subcommand_matches("watch").is_some() && watch(&exercises, verbose).is_ok() {

which then fails silently.

The error of the failing watcher.watch(/*...*/) is Io(Os { code: 28, kind: Other, message: "No space left on device" })' which is most likely a bug in notify as there are hundreds of GB free on the disk. However, I think the error message should at least be propagated and printed to the command line.

Cheers!

@jounathaen
Copy link
Author

The problem lies apparently in the inotify watch limit. All of them were occupied (by my languageclient). So this is not a bug of rustlings. Only the lack of an error message is

@shadows-withal shadows-withal added A-source Area: CLI source C-bug Category: Bug labels Jul 23, 2020
@victordeleau
Copy link

imo I think the auto refresh should be replaced with a manual refresh (press enter refreshes by default for example), and that for 3 reasons:
1- People (like me) might get the issue described here (which I solved by increasing the inotify watch limit of my OS). But we can't expect people to tweak their OS like that to be able to use rustlings.
2- It is bothering when the rustlings watch does its thing without us having a say. Sometimes it is not exactly clear if the error displayed is related to the current exercise or the next one if some modifications made the tests pass. We might also want to read the error again even though we edited the code. That's especially true if the shell running rustlings watch is not in sight of the editor.
3- Pressing enter to refresh is a no-brainer that takes no time.

@seeplusplus
Copy link
Contributor

Looks like this issue may have a resolution soon.

notify-rs/notify#266

@jyn514
Copy link
Member

jyn514 commented Nov 5, 2020

@seeplusplus that issue is just about the error message, there will still be an error either way. The issue here is that rustlings is ignoring the error, which notify can't do anything about.

@jounathaen
Copy link
Author

By the way, if somebody want's to fix this issue. It is probably very easy. The code just has to be changed to something like:

if matches.subcommand_matches("watch").is_some() {
     if let Err(e) = watch(&exercises, verbose) {
        println!("Error: Could not watch your progess. Error message was {:?}", e);
        println!("Most likely you run out of disk space or you 'inotify limit' is reached");
        std::process::exit(1);
    }

I just don't have time to test this properly...

@seeplusplus
Copy link
Contributor

@seeplusplus that issue is just about the error message, there will still be an error either way. The issue here is that rustlings is ignoring the error, which notify can't do anything about.

True. I guess I mean it's solved inasmuch as it responds with a "more correct" error.

I'll try my hand at adding @jounathaen's code and testing it to see if we can resolve this issue.

@seeplusplus
Copy link
Contributor

I just don't have time to test this properly...

For the sake of correctness, what kind of testing would you recommend? I'm not super familiar with testing in Rust (is there some integration test that could be written to test this?). I did test that this gives us the behavior that we want (I dropped the max_user_watches to 256 on my machine and ran "rustlings watch").

@jounathaen
Copy link
Author

That's exactly what I imagined as test.

JuniousY pushed a commit to JuniousY/rustlings that referenced this issue Jan 28, 2021
mccormickt pushed a commit to mccormickt/rustlings that referenced this issue Mar 10, 2021
noiffion pushed a commit to noiffion/rustlings that referenced this issue Aug 20, 2021
bugaolengdeyuxiaoer pushed a commit to bugaolengdeyuxiaoer/rustlings that referenced this issue Dec 28, 2021
ppp3 pushed a commit to ppp3/rustlings that referenced this issue May 23, 2022
dmoore04 pushed a commit to dmoore04/rustlings that referenced this issue Sep 11, 2022
Spacebody pushed a commit to Spacebody/my-rustlings that referenced this issue Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-source Area: CLI source C-bug Category: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants