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

Bitvec as mandatory dependency #1232

Closed
r4v3n6101 opened this issue Nov 5, 2020 · 7 comments
Closed

Bitvec as mandatory dependency #1232

r4v3n6101 opened this issue Nov 5, 2020 · 7 comments

Comments

@r4v3n6101
Copy link

r4v3n6101 commented Nov 5, 2020

What if I wanna use methods of nom's alloc feature, e.g. multi::count and so on, but specifying features = ["alloc"] in Cargo.toml will pull the bitvec crate I don't need. So, there's should be separate option not to include bitvec crate, innit?

@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Nov 21, 2020

There is a bitvec feature, but due to Cargo limitations it's not possible to disable it when using alloc, see rust-lang/cargo#8832.

@decathorpe
Copy link

Yeah, I don't think having bitvec part of the default feature set is a good idea. I'm working on distribution packages for nom, and the new bitvec dependency is proving to be problematic. It introduces at least 5 new dependencies for the default feature set (some of which have only limited support for different CPU architectures), and can't really be disabled, since it's a dependency of the std and alloc features ... if it can't be dropped there, I'll probably have to patch out the bitvec dependency + features entirely - which will cause all sorts of interesting problems if dependent crates start using the bitvec feature ...

@Geal
Copy link
Collaborator

Geal commented Nov 24, 2020

there could be a bitvec-alloc feature to isolate that, it should not be too annoying.

@decathorpe how do distributions usually handle that kind of issue? From my point of view (note that I'm not hostile to distribution packaging, just trying to find a good tradeoff), if a package is created that disables a feature that's part of nom's public API, I'll probably get support requests from people expecting this API. An I already have to work carefully around minimum Rust versions to accommodate Debian packaging.

AFAIK bitvec introduces the following crates: funty, radium, tap and wyz. Looing at the generated Cargo.lock, serde is not used, and none of those dependencies introduce other dependencies.
I assume the target architecture issue comes from radium? Maybe we could patch it to support the architectures you need, that should be enough? paging @myrrlyn for input

@Geal
Copy link
Collaborator

Geal commented Nov 24, 2020

also, correct me if I'm wrong, but is Fedora generating a package per crate? Is there a way to expose the Cargo features as part of the package, to have them configurable by the users, applied transitively, etc?

@decathorpe
Copy link

Thanks for your reply. Yes, radium was the crate I was worried about. For Fedora, I need it to support x86_64, i686, armv7, aarch64, powerpc64le, and s390x. Looking at its build.rs, I can't tell which architectures are actually supported, since it starts off with all features enabled only turns off some or all features if it detects a known not-working architecture - but that doesn't help me figure out which architectures are actually supported yet (since it is not documented). Digging into the Rust source code for those architecture definitions, it looks like they all support the necessary Atomic types up to 64 bits, so that should not be a problem for radium / bitvec. I will try to create packages for the 4 additional dependencies, and if that doesn't work, I will come back.

To give better context regarding Fedora packaging:
Fedora has one "source package" per crate, but from that source, sub-packages for all Cargo feature sets are built, which map the feature dependencies to RPM packages 1:1. In this case, the +default subpackage would depend on +bitvec subpackage, which in turn depends on the RPM package for bitvec+default, and the nom+alloc subpackage would depend on bitvec+alloc, etc. So that gives us the same feature set and granularity as Cargo.

@myrrlyn
Copy link
Contributor

myrrlyn commented Nov 24, 2020

For what it's worth, bitvec is already "maybe broken" on every architecture not named in radium; I just don't know about them because nobody's tried it and told me. bitvec attempts to name atomic symbols and there's no information available from the compiler to prevent me from doing so when those symbols don't exist. radium does not increase the set of assumptions bitvec makes about being able to compile on a given target.


I'm both terrified and intrigued by the concept of creating a system package for bitvec and its subtree. Please do file issues against it and radium with any problems, including architecture compatibility, and I will do my best to help with them.

@decathorpe
Copy link

@myrrlyn Thank you, I will keep eyes open for any issues. For what it's worth, the radium test suite from the last released version seems to pass on all the architectures I care about, so that's a good starting point :)

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

No branches or pull requests

5 participants