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

"watch" is broken on windows #126

Closed
Hades32 opened this issue Mar 18, 2019 · 10 comments · Fixed by #143
Closed

"watch" is broken on windows #126

Hades32 opened this issue Mar 18, 2019 · 10 comments · Fixed by #143
Labels
A-source Area: CLI source C-bug Category: Bug P-high Priority: High

Comments

@Hades32
Copy link

Hades32 commented Mar 18, 2019

The issue seems to be that the call to verify(...) in main.rs:92 expects the path to be in Unix style (forward slashes instead of backslashes), so the later compare breaks.

I fixed this locally by just doing a string replace, but I guess a true Rustacean will fix this much more nicely so I didn't add a patch.

PS: It's also pretty inconvenient that "watch" doesn't continue with the next exercise once you fix the current one.

@komaeda
Copy link
Contributor

komaeda commented Mar 18, 2019

@Hades32 it should continue! sounds like it's broken for you, maybe?

@komaeda komaeda added C-bug Category: Bug A-source Area: CLI source S-waiting-on-author Status: Waiting on issue/PR author labels Mar 18, 2019
@guttume
Copy link

guttume commented Mar 26, 2019

I faced the same issue with watch on Windows

@komaeda komaeda added P-high Priority: High and removed S-waiting-on-author Status: Waiting on issue/PR author labels Mar 27, 2019
@steing
Copy link

steing commented Mar 27, 2019

reverting to tags/1.0.0 and rebuilding seems to have worked for me. compiles on save and advances to next exercise.

@guttume
Copy link

guttume commented Mar 30, 2019

@steing noted. Thank you.

@sebsebmc
Copy link

I have the same issue and #134 did not fix the problem.

bors added a commit that referenced this issue Apr 2, 2019
fix watch command path execution

closes #126

@Hades32 @guttume could you test whether this works on windows by checking out the branch locally and running `cargo run watch`?
@cjpearce
Copy link
Contributor

cjpearce commented Apr 8, 2019

Happy to give fixing this a go if it's still broken. I have a windows vm I can debug in.

@komaeda
Copy link
Contributor

komaeda commented Apr 9, 2019

@cjpearce That would be much appreciated! I'm honestly a bit stuck on this issue as I have neither the time or the resources to get going on this

@sebsebmc
Copy link

sebsebmc commented Apr 9, 2019

I've been debugging the issue and I believe that the problem is that the paths that notify is returning look something like ..../exercises\\variables\\variables1.rs but the value in info.toml looks like exercises/variables/variables1.rs so the ends_with comparison here always fails:

if start_at.ends_with(path) {

I believe the solution may require using something like http://github.com/BurntSushi/same-file

I have a pretty ugly patch that canonicalize()s both paths for comparison and that seems to work on Windows, haven't tested it on Linux yet. I'm hoping there may be a more elegant solution. The other option is to use Path components and checking if the last components match (but I haven't tested if this works yet).

@cjpearce
Copy link
Contributor

cjpearce commented Apr 10, 2019

@sebsebmc That's in line with what I've found too. I think that canonicalize() might be a pretty good solution, since that path really needs it. But I absolutely agree gets pretty messy if we start constructing and then canonicalizing the paths just before we use them.

I think one way of fixing this is to deal with constructing canonicalized the paths up front before we get into any of the logic. I'm working on a PR that would clean up how we handle the paths, which hopefully will mean it doesn't become as messy. I should have it something to show in the next day that we could test.

If you get to a solution you're happy with first I'd be happy to help on that too.

@sebsebmc
Copy link

rust-lang/rust#42869 has a discussion on canonicalization on Windows.

I looked up some projects that use notify and found that alacritty and xi-editor both do canonicalization for their paths.

There is rust-lang/rfcs#2208 that could potentially be used in the future if it ever lands, but it seems like canonicalization should be fine and is expected to work.

bors added a commit that referenced this issue Apr 13, 2019
Canonicalize paths to fix path matching

This PR should fix #126. The main solution to the issue was using `canonicalize()` on the paths we create for the exercises from `info.toml` and any user-specified paths, so that path `ends_with` matching will work correctly.

As adding calls to the canonicalize function everywhere requires unwrapping, I also decided to extract a struct representing an exercise and use serde to deserialize the paths from the `info.toml` file up front. I also tried to move the path handling out into the `exercise.rs` file and down into `main.rs` so that it doesn't create as much clutter. There was already a lot of unwrapping and path handling in the other files and I felt like it was getting a bit too repetitive.

If the approach is going too far (too many changes etc.) I'm happy to try to produce a smaller PR that fixes the bug without any refactoring.
@bors bors closed this as completed in #143 Apr 13, 2019
MrDaar pushed a commit to MrDaar/rustlings that referenced this issue Oct 30, 2019
fix watch command path execution

closes rust-lang#126

@Hades32 @guttume could you test whether this works on windows by checking out the branch locally and running `cargo run watch`?
pedantic79 pushed a commit to pedantic79/rustlings that referenced this issue Apr 11, 2020
…komaeda

Canonicalize paths to fix path matching

This PR should fix rust-lang#126. The main solution to the issue was using `canonicalize()` on the paths we create for the exercises from `info.toml` and any user-specified paths, so that path `ends_with` matching will work correctly.

As adding calls to the canonicalize function everywhere requires unwrapping, I also decided to extract a struct representing an exercise and use serde to deserialize the paths from the `info.toml` file up front. I also tried to move the path handling out into the `exercise.rs` file and down into `main.rs` so that it doesn't create as much clutter. There was already a lot of unwrapping and path handling in the other files and I felt like it was getting a bit too repetitive.

If the approach is going too far (too many changes etc.) I'm happy to try to produce a smaller PR that fixes the bug without any refactoring.
ppp3 pushed a commit to ppp3/rustlings that referenced this issue May 23, 2022
…komaeda

Canonicalize paths to fix path matching

This PR should fix rust-lang#126. The main solution to the issue was using `canonicalize()` on the paths we create for the exercises from `info.toml` and any user-specified paths, so that path `ends_with` matching will work correctly.

As adding calls to the canonicalize function everywhere requires unwrapping, I also decided to extract a struct representing an exercise and use serde to deserialize the paths from the `info.toml` file up front. I also tried to move the path handling out into the `exercise.rs` file and down into `main.rs` so that it doesn't create as much clutter. There was already a lot of unwrapping and path handling in the other files and I felt like it was getting a bit too repetitive.

If the approach is going too far (too many changes etc.) I'm happy to try to produce a smaller PR that fixes the bug without any refactoring.
dmoore04 pushed a commit to dmoore04/rustlings that referenced this issue Sep 11, 2022
…komaeda

Canonicalize paths to fix path matching

This PR should fix rust-lang#126. The main solution to the issue was using `canonicalize()` on the paths we create for the exercises from `info.toml` and any user-specified paths, so that path `ends_with` matching will work correctly.

As adding calls to the canonicalize function everywhere requires unwrapping, I also decided to extract a struct representing an exercise and use serde to deserialize the paths from the `info.toml` file up front. I also tried to move the path handling out into the `exercise.rs` file and down into `main.rs` so that it doesn't create as much clutter. There was already a lot of unwrapping and path handling in the other files and I felt like it was getting a bit too repetitive.

If the approach is going too far (too many changes etc.) I'm happy to try to produce a smaller PR that fixes the bug without any refactoring.
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 P-high Priority: High
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants