Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

remove build-nng from default features #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

M1cha
Copy link

@M1cha M1cha commented Oct 25, 2021

No description provided.

building and linking are things that should be decided by the binary.
Disabling default features is pretty hard though because every single crate
that depends on `nng-sys` would have to use `default-features = false`.

That is quite unrealistic - especially since non-binary crates usually don't
care about this.
@M1cha M1cha force-pushed the disable-build-by-default branch from 3c2a8ee to 61661ca Compare October 25, 2021 12:49
@neachdainn
Copy link
Collaborator

neachdainn commented Oct 25, 2021

I'll assume the commit message is the message you intended for this pull request.

Disabling default features is pretty hard though because every single crate
that depends on nng-sys would have to use default-features = false.

That is quite unrealistic - especially since non-binary crates usually don't
care about this.

As far as I can tell, there is really only a single published crate that directly depends on nng-sys and that crate does use default-features = false while providing its own feature for building the library. This to me suggests that either nng-rs is a direct dependency of the final binary or any layers between the binary and nng-sys are internal. In the former case, this just changes a trivial default that will not genuinely impact anything downstream of nng-rs. In the latter case, requiring that single change in internal libraries is very realistic.

I'm open to seeing some numbers (anecdotal or not) that contradict this, but for the time being I am unconvinced this is a good change. Particularly since I heavily utilize this feature in my own work.

@M1cha
Copy link
Author

M1cha commented Oct 26, 2021

I'll assume the commit message is the message you intended for this pull request.

IMO there's no point in copying the message of the commit into the PR. It's duplicate info and doesn't get updated automatically when I change the commit. Also I might have multiple commits.
Also, the change is where the description should be, not the merge-commit(which might not exist if you use rebase-merge).

Yes I'm using nng-rs but it does not use default-features = false.

In general, yes there's only one crate right now which uses nng-sys but that shouldn't affect the decision of it's design at all given it's a public crate that could be used anywhere.

  • it's easy to enable a feature, but hard to disable one. The reference says the same
  • while I do think that dynamic-linkage to a system library should the default(and most crates do that), that opinion doesn't really matter here because the default could be either one - but without activating a feature. It should have some default with an empty feature list and then there should be a feature which toggles that. So if you want building to be the default you could just rename the feature to e.g. disable-build.

@neachdainn
Copy link
Collaborator

neachdainn commented Oct 26, 2021

IMO there's no point in copying the message of the commit into the PR. It's duplicate info and doesn't get updated automatically when I change the commit. Also I might have multiple commits.

It should provide a summary of the changes you're asking to merge, if for no other reason than to be polite. If the merge request was any larger, I would probably put it on the back burner simply because I don't want to have to dig just to get a summary of what is going on.

Yes I'm using nng-rs but it does not use default-features = false.

You're right. Apparently I messed that up when I upgraded nng-sys versions. I'll have to fix that.

Overall, there are more significant issues with this crate that prevent this change from being meaningful. The versioning scheme alone is causing problems (@jeikabu I will probably switch it - we can discuss). Additionally, the primary consumer of the crate completely negates this change anyway.

That being said, if you can fix the CI and open a similar MR to nng-rs, I'll consider merging it. Things need to be fixed anyway.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants