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

glob.h bindings #5832

Closed
wants to merge 3 commits into from
Closed

glob.h bindings #5832

wants to merge 3 commits into from

Conversation

doy
Copy link
Contributor

@doy doy commented Apr 11, 2013

This adds low level bindings for glob() and globfree() in libc, and adds a static glob() function to Path which uses those. It has only been tested on Linux/x86_64, but I looked up header file definitions for other platforms, so they should hopefully work in theory. Windows doesn't have these functions, so that is currently unimplemented, although ideally Path::glob() could be reimplemented in terms of Windows primitives instead, at some point in the future.

I'm not sure what a good way to test this would be - it requires looking at the actual filesystem, so I'm not sure where a safe place to do that would be. I'd be willing to add some if people have recommendations.

@erickt
Copy link
Contributor

erickt commented Apr 11, 2013

I'm not sure if this should be in the standard library, but instead in a third party crate. What does everyone else think?

@bstrie
Copy link
Contributor

bstrie commented Apr 12, 2013

@erickt I don't know about systems languages, but globbing facilities are certainly present in the standard libraries of scripting languages.

I'm ever-so-slightly concerned that, in the absence of this, people will resort to shelling-out to echo *.foo rather than installing a third-pary lib.

@graydon
Copy link
Contributor

graydon commented Apr 12, 2013

I think it's fine to adopt something like this. I'm uncomfortable about this as-presented for the following reasons:

  • I think it'd be nice (though not necessarily easy) to encode more-portable logic in rust itself; I'm concerned that this may be an OS, charset and libc variable function? Calling libc for this seems a bit awkward.
  • I think it'd be nice to support a stronger version that uses, say, regexps (not that we have a regexp library yet either). Globs are quite simple.
  • The current code here always allocates, whereas I'd like it to provide callbacks with slices if it's got pre-allocated strings already. Unless, I guess ... hm, difficult design choice: currently Path as designed owns its string components, so I guess it's congruent. But it might be worth revisiting, now that we have working borrowed pointers? (we did not really when Path was created)
  • The current code places an imperative search in the path module. So far we've put operations that touch the filesystem in os and reserved the path module for just pure operations on the in-memory type representing paths.

@doy
Copy link
Contributor Author

doy commented Apr 13, 2013

  • Yes, I agree - long term at least, the user-facing version should probably not use the libc glob functions, if for no other reason than portability (we'll have to include an internal version for windows support anyway, so there's not really any reason for us to not use that internal implementation for all platforms). This was really just to start trying out what an API for this would look like - I do think that something like this is something that we want in the standard library, and using the POSIX glob functions seemed like the easiest way to get something implemented. Do you think it's worth including a preliminary version using this sort of implementation, with the intention of replacing it with something better later? Or do you want to hold off for a better implementation?
  • Adding more features would probably be good too. I don't know that we need a full regex thing, but Ruby's glob implementation for instance supports ** for recursive matches. Having the capability to do things like that would be nice.
  • That's probably reasonable, although no reason that we couldn't provide both.
  • Sounds fine to me.

@graydon
Copy link
Contributor

graydon commented Apr 13, 2013

Ok. Can you move the code to the os module, add a comment / bug improve it in the ways noted, and refresh? I'd be ok landing it given that.

@doy
Copy link
Contributor Author

doy commented Apr 13, 2013

Updated. I'll file a bug for the todo items once it gets merged.

@graydon
Copy link
Contributor

graydon commented Apr 15, 2013

sadly now this needs a rebase :(

only tested on linux/x86_64, but i got the values for other platforms
from their system header files.

no bindings for win32, because win32 doesn't include glob.h.

also, glob() takes a callback for error handling, but i'm just making
this a *c_void for now, since i don't know how to represent c calling
back into rust (if that's even currently possible).
this could probably use expansion - it just uses all of the default
options, which is usually what we want, but not always. maybe add a
separate function that takes more options?
@doy
Copy link
Contributor Author

doy commented Apr 29, 2013

Okay, this is updated. I'm seeing a failure in the test suite in src/test/run-pass/too-much-recursion.rs: "rust: task 7f00c02041b0 ran out of stack", but I don't think it's related to my changes.

@doy
Copy link
Contributor Author

doy commented May 1, 2013

Oops, forgot to add pub on the type for all platforms. Should be fixed now.

@bors bors closed this May 1, 2013
@graydon graydon reopened this May 1, 2013
@graydon graydon closed this May 1, 2013
@graydon graydon mentioned this pull request May 1, 2013
bors added a commit that referenced this pull request May 1, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 26, 2020
…earth

Update Usage section of README.md

Fixes rust-lang#5826

changelog: none
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