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

nu 0.60 beta cannot ls in a UNC share #4863

Closed
anisoptera opened this issue Mar 17, 2022 · 17 comments
Closed

nu 0.60 beta cannot ls in a UNC share #4863

anisoptera opened this issue Mar 17, 2022 · 17 comments
Labels
🐛 bug Something isn't working help wanted Extra attention is needed
Milestone

Comments

@anisoptera
Copy link

Describe the bug

This works on 0.44.0 (released non-beta). If you cd to a UNC share in the 0.60.0 beta, ls doesn't work. Instead of listing files I get an error about ./*: not found.

I can still cd to directories I know exist, but listing files fails no matter what directory I'm in on the share.

How to reproduce

  1. cd to a UNC share (\servername\sharename)
  2. type ls

Expected behavior

List files in a fancy table

Screenshots

No response

Configuration

No response

Additional context

No response

@anisoptera
Copy link
Author

Hmm, on digging slightly deeper, even on nushell 0.44.0 the UNC support is not great:

\\?\UNC\filez\media\movies> ls convert.zsh
───┬─────────────┬──────┬───────┬───────────
 # │    name     │ type │ size  │ modified
───┼─────────────┼──────┼───────┼───────────
 0 │ convert.zsh │ File │ 703 B │ a day ago
───┴─────────────┴──────┴───────┴───────────

\\?\UNC\filez\media\movies> cp convert.zsh convert.nu
error: No matches found
   ┌─ shell:19:4
   │
19 │ cp convert.zsh convert.nu
   │    ^^^^^^^^^^^ no matches found

@fdncred
Copy link
Collaborator

fdncred commented Mar 17, 2022

I'm betting that our nu-path crate doesn't understand UNC yet. hmmmmm, after looking a bit, it used dunce which should work with UNC. so maybe something else is going on.

@fdncred fdncred added the 🐛 bug Something isn't working label Mar 17, 2022
@fdncred
Copy link
Collaborator

fdncred commented Mar 17, 2022

this seems to work

> ls \\localhost\admin$ 
╭─────┬──────────────────────────────────────────────────────────────────┬──────┬──────────┬────────────────╮
│  #  │                               name                               │ type │   size   │    modified    │
├─────┼──────────────────────────────────────────────────────────────────┼──────┼──────────┼────────────────┤
│   0 │ \\localhost\admin$\AppReadiness                                  │ dir  │      0 B │ 44 minutes ago │
│   1 │ \\localhost\admin$\Boot                                          │ dir  │   4.1 KB │ 9 months ago   │
│   2 │ \\localhost\admin$\Branding                                      │ dir  │      0 B │ 9 months ago   │
│   3 │ \\localhost\admin$\BrowserCore                                   │ dir  │      0 B │ 9 months ago   │
│   4 │ \\localhost\admin$\CSC                                           │ dir  │      0 B │ 2 years ago    │
│   5 │ \\localhost\admin$\CbsTemp                                       │ dir  │   4.1 KB │ a week ago     │
│   6 │ \\localhost\admin$\Containers                                    │ dir  │      0 B │ 9 months ago   │

but if i do this cd \\localhost\admin$ and then try ls i get:

Error:
  × No matches found for ./*
  help: no matches found
> $env.PWD
\\?\UNC\localhost\admin$
> ls $env.PWD
Error:
  × No matches found for \\?\UNC\localhost\admin$\*
  help: no matches found

@fdncred
Copy link
Collaborator

fdncred commented Mar 17, 2022

I think this is the problem. Our glob crate doesn't know what to do with VerbatimUNC paths.

if root_len > 0
&& check_windows_verbatim(root.expect("internal error: already checked for len > 0"))
{
// FIXME: How do we want to handle verbatim paths? I'm inclined to
// return nothing, since we can't very well find all UNC shares with a
// 1-letter server name.
return Ok(Paths {
dir_patterns: Vec::new(),
require_dir: false,
options,
todo: Vec::new(),
scope: None,
});
}

unc path

root=[Some("\\\\?\\UNC\\localhost\\admin$\\")]

path components

p=PrefixComponent { raw: "\\\\?\\UNC\\localhost\\admin$", parsed: VerbatimUNC("localhost", "admin$") }

@MCoooo
Copy link

MCoooo commented Jun 8, 2022

Hi, this issue is caused by the PWD using UNC. If PWD is set to drive format, e.g. let-env PWD = "L:\Scripts" , all is well.

nushell-pwd

@anisoptera
Copy link
Author

Yes, the bug is specifically about the pwd being a UNC path. I don’t want to have to map a drive letter to use a share. I can’t switch away from powershell without feature parity :)

@MCoooo
Copy link

MCoooo commented Jun 10, 2022

Ah sorry - reading helps! I raised a similar issue but for drive map use case but didn't pick up on this distinction (folded into here as dup).

While being very much on at the start of my Rust (and GitHub) journey and without understanding the implication, but wishing to use nu on a daily basis, I've forked and and swapped \\?\UNC\ to \\ in cwd in env.rs.

Hasn't broken yet. If of any use...

https://github.com/MCoooo/nushell_hack

Hopefully get time to understand what's going on more fully

@MCoooo
Copy link

MCoooo commented Jun 13, 2022

After removing the condition
'if root_len > 0
&& check_windows_verbatim(root.expect("internal error: already checked for len > 0"))'

and using a verbatim path, nu ls does generate a contents list but not as expected. When change directory uses a verbatim path some auto correction takes place where needed (device to unc and case for example). When ls is used directly against a verbatim, there is no autocorrection, however it does generate a list.

However verbatim paths currently do no just display the files but some of the path, so I assume the 'name' object is incorrect for use in piped commands.

Not sure of the QA benchmarks for changes, but I would argue its better to promote adoption in the Windows (Enterprise) space with this limitation / rough edge. Unless of course more is broken...

allow_verbatim

@charlesroper
Copy link

I came across this issue trying to CD into WSL. My Ubuntu WSL instance is mapped to U: but the path gets converted to a UNC path.

2022-08-13_01

There's an open issue about this for zoxide: ajeetdsouza/zoxide#162

Which leads to a problem with dunce: https://gitlab.com/kornelski/dunce/-/issues/2

Could this be the problem?

@fdncred
Copy link
Collaborator

fdncred commented Aug 13, 2022

cd is not the same as ls, of course, but this problem could be related. we use dunce as well but switching from dunce to crate x may not solve the problem this issue states (but it may solve the cd'ing issue). I spelled out above what I thought the issue was with verbatimunc paths.

@charlesroper
Copy link

Sorry, I wasn't clear! cd'ing works. The error you see in my screenshot is Zoxide failing (maybe because of dunce). I can, however, still type commands. If I invoke ls, it gives me the error this issue is about.

2022-08-14_01

Not sure if these things are related or not, but seemed worth mentioning.

@fdncred
Copy link
Collaborator

fdncred commented Aug 15, 2022

i'm guessing they're related but someone would have look at the ls code and debug it probably.

@ehawman
Copy link

ehawman commented Sep 3, 2022

Wanted to chime in by saying that recently I've been getting a more aggressive error message.

image

Not sure if this is due to some third-party application or if there are just more debug options in Nu now.

I should note here that I have pwd as

def pwd [] {
    echo $env.PWD | sd \\ /
}

Hence the forward slashes, but both manual entry and cd pwd results in the same gobbledygook.

@fdncred
Copy link
Collaborator

fdncred commented Sep 3, 2022

@ehawman That's weird. Those look like dotnet errors. nushell doesn't have any dotnet but on Windows it may call into the Windows API that is dotnet.

@rgwood
Copy link
Contributor

rgwood commented Oct 23, 2022

Good news, UNC issues should be fixed by #6824. The fix will be released in Nushell v0.71.

@rgwood rgwood closed this as completed Oct 23, 2022
@hustcer hustcer added this to the v0.71.0 milestone Oct 23, 2022
@MCoooo
Copy link

MCoooo commented Oct 23, 2022

Fantastic. Great work all.

@charlesroper
Copy link

Thank you so much folks! Especially @ChrisDenton for omnipath. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants