Use opt-in features instead of opt-out

[m4b]

Could you clarify this more?

I'm considering switching, but I'm wondering what the/your use case might be, and why it's better.

For example, it seems that the most common use of the crate is to use everything, and more specialized consumers will opt out, but this could be wrong.

I'd suggest making a poll but I'm not sure enough people use the crate for it to be unbiased ;)

[hauleth]

You use flags like elf, elf64 to enable support for them, and then add default feature that enables them all. This is generally preferred way to use feature flags than opt-out of features.

Łukasz Niemier lukasz@niemier.pl

Wiadomość napisana przez m4b notifications@github.com w dniu 21.09.2016, o godz. 11:22:

Could you clarify this more?

I'm considering switching, but I'm wondering what the/your use case might be, and why it's better.

For example, it seems that the most common use of the crate is to use everything, and more specialized consumers will opt out, but this could be wrong.

I'd suggest making a poll but I'm not sure enough people use the crate for it to be unbiased ;)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/m4b/goblin/issues/8#issuecomment-248557463, or mute the thread https://github.com/notifications/unsubscribe-auth/AARzN2XrTghE8zYx7uIvEszII04gj8viks5qsPc5gaJpZM4KCkU1.

[m4b]

Can you provide a simple PR demonstrating this?

Iirc there was some technical reason for using opt-out w.r.t. dryad, but it escapes me at the moment. Perhaps time to get this straightened out before 1.0?

Also I think no_endian_fd will likely remain the same since the default should be endian aware reading unless otherwise specified.

[eternaleye]

There are two distinct concerns here:

1. Opt-in vs. opt-out in terms of "what the default is"
2. Opt-in vs. opt-out in terms of "what the polarity of each feature is"

These are orthogonal, and actually have very different behaviors:

Opt-in vs. opt-out as far as defaults is a matter of preference - generally speaking, if something has no dependencies and no conflicts or concerns, it's often nicer to downstream to enable it by default,

Opt-in vs. opt-out regarding feature polarity, however, has exactly one answer: Opt-in. Anything else breaks Cargo. The reason is that Cargo assumes that it is always safe to turn on a feature, and that doing so will never break a dependent. As a result, features with opt-out polarity will get enabled if any dependent asks for them, breaking anyone who uses the now-missing APIs.

The correct approach:

Cargo.toml:

[features]
default = ["elf", "elf64", "..."]

• Anything in default is on by default
• Anything not in default is off by default
• Crates that don't need all of the default features can opt out of the default set and choose them á la carte
• Crates that need non-default features can still enable them
• Nobody's build gets broken.

But also, beware: Features that change the behavior of an API - like you seem to describe no_endian_fd - are always unsafe, because if one dependent flips them, that affects every dependent.

Those kinds of features need to be separate APIs, under different names or modules, with the feature simply enabling them.

[m4b]

@eternaleye Hey, thank you so much for the detailed write up. Your arguments are compelling, and you convinced me to switch, with some caveats below.

The unused boolean in the non endian from_fd always bothered me from a usability perspective, and combined with #10, I am thinking we rename the endian aware from_fd API to just parse<Read + Seek> to allow some awesome unit testing and better flexibility. This will not use byteorder unless the consumer specifies default or the feature flag (a requirement for dryad)

That being said, I'd like to get your opinion on the pure flag.

Now by definition, this flag disables APIs. There are a few other crates that perform similarly, byteorder being one. (It actually does the inverse, so no default turns it off, but that crate is substantially less complicated than goblin's ELF module)

Before I (or someone else) begins to port the flags to the proposal here, I think a PR demonstrating feasibility or how the no_std/pure version will work, will be required.

So some hard, non negotiable requirements for the feature flag port:

1. Redox must be able to use goblin in a no std setting with all the API not in the hidden impure module (the makefile flags can obviously be changed), with no source changes to the current usage of goblin's API, excepting the from_fd removing the boolean (I don't think it uses that iirc, but if it does). Ie, you're only allowed to change how goblin builds, the current API will be backwards compatible, except the from_fd function
2. Dryad must compile and have access to the non endian reader (ideally, with as little relocations as possible...), and cannot use/compile the byteorder crate... Ideally also dryad at compile time selects either the elf32 or elf64 depending on the platform
3. Panopticon must compile and have access to endian fd readers and all the binary modules (this is easiest and iirc will just be the default setting)

If this is possible, I'm OK with the feature flag change.

My suspicion is that these requirements are much harder than they seem, but the current reverse polarity setup satisfies all of them...

[eternaleye]

Regarding pure, I had a similar discussion on https://github.com/briansmith/ring/pull/256

For (1), it's the exact same situation as #![no_std] itself - every crate in the Redox dependency graph that depends on goblin would need to opt-out of the defaults with goblin = { version = "...", default-features = false }, just like they need to opt out of std. (This assumes impure is a default feature; if it's not, then they simply need to not use it).

For (2), the "cannot use byteorder" constraint would mean it should opt out of default features to disable the endian-aware one, and then it would just explicitly enable the appropriate feature flag for the reader. Again, it must ensure that no other crate in the graph depends on goblin with the default features enabled.

(3) is trivial, yeah.

Of course, all three of these are top-level crates, and so have solid control over their dependency graphs. That makes it relatively easy.

While ring's use would be enhanced by either target-specific features or scenarios, I think your use case would benefit most from the latter, allowing "fail-fast" on options being enabled when they are not viable.

[m4b]

@eternaleye After reading your discussion on the other thread, and your suggestions above, I'm in complete agreement.

In fact, I think this should be made much more evident in the cargo docs for feature flags, as the seriousness of using negative polarities isn't nearly crucial enough imho, in addition to many people seem to be implementing the negative polarity approach for whatever reason.

Anyway, thanks for your stewardship and help on this matter, I'm glad this was resolved before a serious 1.0 release :)

[eternaleye]

Glad to help!

And yeah, I agree the Cargo docs really ought to be more explicit about this.