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

Add command line globbing to all utilities (for windows targets) #1281

Merged
merged 14 commits into from
Feb 7, 2019

Conversation

rivy
Copy link
Member

@rivy rivy commented Aug 29, 2018

Command line globbing is added (for windows targets) to all utilities via 'uucore'.

This change increases portability and parity across machine targets.

This initial commit uses the published 'wild' crate. The second commit references a personal fork of 'wild' which updates the API and includes proper case insensitivity (again, for windows targets). I plan to update and re-reference the published 'wild' crate when it is updated to include the fixes.

@rivy
Copy link
Member Author

rivy commented Sep 1, 2018

It's not obvious how to fix the failing tests from my end.

I do believe they are due to bugs in wild and/or glob, but glob seems semi-deprecated and wild is hard for me to grok in order to issue a PR for repair of the problem (maybe the author of wild could offer a hand? @kornelski?).

Maybe the three failing tests should just be gated out for Windows for the time being until the globbing story becomes more polished (see rust-lang/glob#59).

@kornelski
Copy link

kornelski commented Sep 2, 2018

Note that Windows' quoting/escaping syntax is bizarre, and incompatible with any sensible Unix-like expectations. It has weird hacks to tolerate unescaped backslashes, and weird cases where:

exe """"""""

is parsed as "" (i.e. "\"\"").

Also keep in mind that cargo run doesn't preserve quoting, so if you test anything that way it'll give wrong results. Only executables launched directly without cargo have access to unparsed command line.

@rivy
Copy link
Member Author

rivy commented Sep 3, 2018

Agreed, CMD has a LOT of weird edge cases. But the initial failing test that I see is echo \\ returning just a single \. No quotes involved. In fact, when I tested the executable with various permutations of slashes, it will "compress" them.

C:> REM "compression" of slashes
C:> uutils echo \
\
C:> uutils echo \\
\
C:> uutils echo \\\\\\
\
C:> uutils echo \\.
\.
C:> uutils echo \\\\.
\.
C:> uutils echo /
/
C:> uutils echo ///////
/
C:> uutils echo //.
/.
C:> uutils echo ////.
/.
C:> REM but if not a possible PATH? ...
C:> uutils echo \\x //x
\\x //x

I think the wildcard/globbing process is being overly aggressive. If there aren't any glob characters in an argument (or if globbing returns an empty set), the argument should just be returned. Isn't that a wild/globbing issue?

@kornelski
Copy link

Oh, I see. Fair point about overly-aggressive glob. I'll check that out.

@kornelski
Copy link

Fixed in wild v1.1.1 and v2.0.1

@rivy
Copy link
Member Author

rivy commented Sep 3, 2018

👍

Looking better, and passing all tests, on my machine... Let's see how we do with AppVeyor.

@rivy
Copy link
Member Author

rivy commented Sep 3, 2018

Excellent, everything is passing, thanks @kornelski !

src/uucore/lib.rs Show resolved Hide resolved
@vks
Copy link
Contributor

vks commented Sep 10, 2018

This is problematic, because * is a valid file or folder name on Unix. If I understand correctly, this PR makes it impossible to remove such files and folders.

Only enabling the feature on Windows might work, because there * is not allowed in file and directory names.

@kornelski
Copy link

kornelski commented Sep 10, 2018

The wild crate is a no-op on everything but Windows (on Unix it re-exports std::env::args() and that's it).

@rivy
Copy link
Member Author

rivy commented Oct 6, 2018

@Arcterus , I've got a branch with rust@1.27 and revised / working Travis CI testing at https://github.com/rivy/rust.coreutils/tree/fix.ci, specifically rivy@deead6f. It includes AppVeyor changes as AppVeyor CI was failing for odd reasons; so I remodeled it based upon a newer template (see https://github.com/starkat99/appveyor-rust/blob/master/appveyor.yml).

The full comparison is master...rivy:fix.ci.

The Travis CI build is at https://travis-ci.org/rivy/rust.coreutils/builds/432010513.

The AppVeyor CI build is at https://ci.appveyor.com/project/rivy/rust-coreutils/build/2%20~%20fix.ci.

I can add all or part of it to this PR... let me know how you'd like me to proceed.

(( reposted from sub-discussion above ))

@Arcterus
Copy link
Collaborator

It’s fine if you include it in this PR.

@rivy
Copy link
Member Author

rivy commented Oct 15, 2018

I've used this updated PR as a base for further fixes, and it seems like a good new base.

All tests are working correctly for all of my newer commits (which I'll rebase and PR after this is merged).

@cnd
Copy link
Contributor

cnd commented Oct 19, 2018

@Arcterus can we merge this?

@rivy
Copy link
Member Author

rivy commented Nov 3, 2018

Ping @Arcterus.

Anything changes needed before a merge?

I've got a couple of other pending fixes based on this commit set.

@rivy
Copy link
Member Author

rivy commented Nov 24, 2018

@Arcterus ,

Just FYI, I've got several pending PRs based on this now (#1304, #1305, #1315, #1316, #1317, #1318).

@rivy
Copy link
Member Author

rivy commented Jan 7, 2019

@Arcterus @cnd

I just wanted to check in now that the holidays are over and see if we can get this merged (as well as the other PR's mentioned above) ...

@rivy
Copy link
Member Author

rivy commented Jan 12, 2019

@vks ?

@Arcterus
Copy link
Collaborator

Arcterus commented Feb 7, 2019

Really sorry about being MIA for the past while. This seems good to me though!

@Arcterus Arcterus merged commit 5a17daa into uutils:master Feb 7, 2019
@rivy rivy deleted the alt/win-cli-globbing branch March 12, 2019 18:54
Arcterus added a commit to Arcterus/coreutils that referenced this pull request Jun 18, 2020
Add command line globbing to all utilities (for windows targets)
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.

5 participants