Skip to content

Introduce use_std default feature #421

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

Closed
wants to merge 1 commit into from
Closed

Conversation

vmx
Copy link

@vmx vmx commented Nov 29, 2017

regex depends on memchr and aho-corasick which can optionally be compiled
without std. With the introduction of the use_std feature it is now possible
to compile regex with default-features = false which then uses a memchr
and aho-corasick that don't depend on std.

This makes regex available on platforms like wasm which haven't a libc
implemented.

regex depends on memchr and aho-corasick which can optionally be compiled
without std. With the introduction of the `use_std` feature it is now possible
to compile regex with `default-features = false` which then uses a memchr
and aho-corasick that don't depend on std.

This makes regex available on platforms like wasm which haven't a libc
implemented.
@vmx
Copy link
Author

vmx commented Nov 29, 2017

Please note that this pull request needs a new release of aho-corasick which just merged the corresponding commit.

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some concerns in the comments.

@alexcrichton Does this line of action make sense with our wasm story? (I don't really have much context on wasm.)

@@ -19,9 +19,9 @@ appveyor = { repository = "rust-lang-libs/regex" }

[dependencies]
# For very fast prefix literal matching.
aho-corasick = "0.6.0"
aho-corasick = { version = "0.6.0", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is blocked on aho-corasick getting a new release. regex would then need to depend on at least the newly released version.

# Enable to use the unstable pattern traits defined in std.
pattern = []
# Enable to use simd acceleration.
simd-accel = ["simd"]
# Allow compilation of dependencies without std
use_std = ["aho-corasick/use_std", "memchr/use_std"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now that I'm thinking about this (and this comment applies to the aho-corasick crate as well), I'm thinking that use_std might not be an appropriate name. It's surely appropriate for memchr since the absence of use_std actually means that memchr won't use std at all. But in the case of aho-corasick and regex, that's not what use_std means at all. Instead the absence of use_std in regex and aho-corasick seems to mean that it won't explicitly import from libc anywhere, which is quite a bit different than not using std.

I wonder if it makes sense to add a use_libc feature instead (and similarly for aho-corasick). That would then in turn have to imply the memchr/use_std feature.

@andrew-d
Copy link

andrew-d commented Nov 30, 2017

(edited to remove offtopic stuff - my apologies, I misread the initial issue)

@BurntSushi
Copy link
Member

@andrew-d Uh, could you please open another issue? What you're talking about seems completely orthogonal to the problem that @vmx is trying to solve.

Spoiler alert: allocation is weaved into the very guts of regex, and I don't see that ever changing, so the very best you'll be able to do is no_std but with alloc. A true no_std or core-only regex library is a special beast that should, at the very least, be prototyped out-of-tree by someone else. :) --- I'm happy to discuss this more if you have more thoughts, but please open another issue.

@alexcrichton
Copy link
Member

Interesting! I think for something like wasm it's probably best to "work by default" rather than detect use_std or not though perhaps?

@BurntSushi
Copy link
Member

Could you unpack that? I think you're saying that memchr should make libc a dep for every target except wasm?

@alexcrichton
Copy link
Member

Oh sorry sure yeah. So in general if it can every crate I think should compile every platform by default. For example that means that memchr wouldn't have a libc dep on the wasm platform (but it would on Linux for example). That way you don't have to worry about fiddling with features for portability, but rather the crates choose a "reasonable default" and try to be portable by default.

Similarly the regex crate would in theory compile-by-default for wasm (for whatever definition that may entail).

It's tricky when a platform doesn't present some functionality though, but thankfully I think that's not the case for wasm?

@BurntSushi
Copy link
Member

BurntSushi commented Nov 30, 2017

@alexcrichton Ah OK, I think I just completely forgot about specifying target specific dependencies. I agree that that seems like what we should do here. In this case, memchr has fallbacks in the absence of libc so it's just a matter of setting the right cfgs I think.

@vmx So I think the solution to your problem here is:

  1. Rollback the change to aho-corasick.
  2. Make libc a target specific dependency.

For (2), the docs for that I think are here: http://doc.crates.io/specifying-dependencies.html#platform-specific-dependencies --- So I'd guess you'd want (assuming not(target_arch = "wasm") is actually the right thing here):

[target.'cfg(not(target_arch = "wasm"))'.dependencies]
libc = { version = "0.2.18", default-features = false, optional = true }

You'll also need to mess with the cfgs inside memchr itself. You probably need to change each cfg(feature = "libc") to cfg(all(feature = "libc", not(target_arch = "wasm"))).

And then we shouldn't need to change regex or aho-corasick at all.

@vmx
Copy link
Author

vmx commented Nov 30, 2017

@BurntSushi Thanks for the detailed explanation. I'll take a stab.

@alexcrichton
Copy link
Member

Oh to clarify I woudln't worry about target-specific dependencies in the sense that libc compiles on the wasm target it's just empty, so you'll just need to #[cfg] internally to not actually use libc

@vmx
Copy link
Author

vmx commented Nov 30, 2017

There it is: BurntSushi/memchr#23
It works locally but needs a few new releases of various other crates.

@vmx vmx mentioned this pull request Nov 30, 2017
@vmx
Copy link
Author

vmx commented Nov 30, 2017

I'm closing this one in favour of #422, which is a lot more on-topic.

@vmx vmx closed this Nov 30, 2017
@vmx vmx deleted the use-std-feature branch November 30, 2017 14:06
vmx added a commit to vmx/aho-corasick that referenced this pull request Nov 30, 2017
The intention for this change was to get aho-corasick
compiled on wasm. Thanks to the discussion at
rust-lang/regex#421
it became clear that there's a better solution to the
problem.

This reverts commit 0440919.
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.

4 participants