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.

Sign up for GitHub

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

Jump to bottom

treewide: remove lib.meta.enableIfAvailable #40255

Merged
merged 1 commit into from May 10, 2018
Merged

treewide: remove lib.meta.enableIfAvailable #40255

merged 1 commit into from May 10, 2018

Conversation

matthewbauer
Copy link
Member

Motivation for this change

This seems like a better approach than "enableIfAvailable" to me. The issue with "enableIfAvailable" is it combines two concepts: optional dependencies (usually configured by a flag) and availability, that are better left separate. This may seem convenient but it gives users less control over what packages are being built. For both packages (Nix & Systemd) using "enableIfAvailable" right now, it's pretty clear that they are being used as we normally use optional dependencies. Both "libseccomp" & "kexectools" can be left out and they will run correctly, whether each is broken or not. So, it makes more sense to make these configurable with a flag. We can still provide libseccomp.meta.available & kexectools.meta.available as defaults.

I don't know if anyone has strong feelings in favor of enableIfAvailable. I definitely think it's an overloaded concept that shouldn't be used extensively in Nixpkgs. If necessary packages can always define their own version of enableIfAvailable, but I definitely don't think it should be included in lib.

@matthewbauer matthewbauer requested a review from shlevy May 9, 2018 21:28
@shlevy
Copy link
Member

shlevy commented May 10, 2018

@Ericson2314

@Ericson2314
Copy link
Member

Ericson2314 commented May 10, 2018 •
edited

@matthewbauer the tl;dr as long as we keep on switching to ? meta.availible and no null hacks, I'm good.

The idea behind enableIfAvailable is sort of "anonymous private use flags", indeed conflating things as you say. The crucial and unwritten invariant (that I just thought of :D) is the optional dep shouldn't affect the public interface, that way we know know other package need care. You can think of it as sort of the opposite end of the importance scale compared to your top-level use-flags idea.

But @dezgeg had some concerns that this was perhaps easy to abuse (say a supposed "optional" dep is actually "conditionally mandatory" on certain platforms, i.e. a sort dependent not independent), and now this would be the second time I'd be back-pedaling on where it should be used, so maybe its just better to push people towards things like enableIfAvailable to cut boilerplate.

Merge if you like :).

@matthewbauer matthewbauer merged commit 4ec9c4b into NixOS:master May 10, 2018
@dezgeg
Copy link
Contributor

dezgeg commented May 11, 2018 •
edited

I disagree with this foo.meta.available thing if it means specifying config.allowUnsupportedSystem effectively breaks nixpkgs by making it do unpredictable things (which is what this enableIfAvailable) did.

Some Darwin user on IRC already asked/complained why they can't build nix anymore when they enable allowUnsupportedSystem as it started to build libseccomp and then failed.

This behaviour seriously should be reverted.

@oxij
Copy link
Member

oxij commented May 12, 2018 via email

@matthewbauer matthewbauer deleted the remove-enableIfAvailable branch February 22, 2019 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shlevy shlevy Awaiting requested review from shlevy

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@matthewbauer @shlevy @Ericson2314 @dezgeg @oxij @GrahamcOfBorg