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

haskell: make generic builder follow compiler’s shared config #42311

Merged

Conversation

matthewbauer
Copy link
Member

enableShared in generic-builder.nix should default to what the GHC
compiler was compiled with. Add a passthru to all of the GHC compilers
to hold the value of enableShared. If enableShared is not set in the
GHC we just use false as the default value for enableSharedLibraries.

Note: I may have missed some compilers. Only GHC & GHCJS are covered
by this commit but this shouldn’t break evaluation of anything else.

@matthewbauer matthewbauer requested a review from peti as a code owner June 20, 2018 22:11
@GrahamcOfBorg GrahamcOfBorg added 6.topic: haskell 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jun 20, 2018
@Ericson2314 Ericson2314 force-pushed the haskell-enable-shared-defaults branch from ebf722c to 80b7eb0 Compare June 20, 2018 22:39
@Ericson2314
Copy link
Member

Added 8.2.1-binary

enableShared in generic-builder.nix should default to what the GHC
compiler was compiled with. Add a passthru to all of the GHC compilers
to hold the value of enableShared. If enableShared is not set in the
GHC we just use false as the default value for enableSharedLibraries.

Note: I may have missed some compilers. Only GHC & GHCJS are covered
by this commit but this shouldn’t break evaluation of anything else.
@Ericson2314 Ericson2314 force-pushed the haskell-enable-shared-defaults branch from 80b7eb0 to fd7a6ea Compare June 20, 2018 22:41
@peti
Copy link
Member

peti commented Jun 21, 2018

enableShared in generic-builder.nix should default to what the GHC compiler was compiled with.

Why should it?

@matthewbauer
Copy link
Member Author

matthewbauer commented Jun 21, 2018

Why should it?

My understanding is that if your compiler wasn't compiled with "enableShared", setting "enableSharedLibraries" to true, will break things.

@Ericson2314
Copy link
Member

enableSharedLibraries ? (ghc.isGhcjs or false) || stdenv.lib.versionOlder "7.7" ghc.version)

is always true today. I gather this is because old GHC didn't support dynamic linking, so that reads "true where possible". If anyone has a custom compiler that doesn't support dynamic linking, they can do passthru = { enableShared = false; }; just like with the prebuilt ones. If we want to add additional conditions we can do

enableSharedLibraries ? ghc.enableShared or false && additionalCondition

The point being deduplicating the logic like this doesn't tie our hands.

@Ericson2314 Ericson2314 merged commit e050011 into NixOS:master Jun 21, 2018
@Ericson2314 Ericson2314 deleted the haskell-enable-shared-defaults branch June 21, 2018 22:07
@peti
Copy link
Member

peti commented Jun 22, 2018

My understanding is that if your compiler wasn't compiled with "enableShared", setting "enableSharedLibraries" to true, will break things.

I see an argument to be made that you'd want things to break in that case. The enableShared default in the generic builder can be viewed as expressing a policy that compilers ought to fulfill and if someone uses the generic builder with a compiler that does not fulfill that policy then you ought to pass a proper enableShared setting along your custom compiler. That approach makes these choices more explicit, whereas the new scheme that @Ericson2314 chose to merge without waiting for my review makes this choice implicit.

@matthewbauer
Copy link
Member Author

Feel free to revert if you disagree. This was motivated by the need to have "enableSharedLibraries" set to false in prebuilt Android & iOS (which is the default for their config) and they are broken otherwise. I felt like this approach would avoid the most duplication but there are certainly other ways to accomplish this. If you do want to revert I hope it is okay to add an extra condition to enableSharedLibraries in generic-builder.nix.

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 22, 2018

Yeah we can revert this in the interim, but let me continue to argue for this approach.

@peti so first of all, are you OK with us adding the cross exceptions to the generic builder default? If we do, then the default becomes a verbose condition that doesn't to me express a policy other than "build shared libraries where possible". (Any other interpretation would strike me as an "overfit" one.) If we don't, then users must manually instruct generic-builder even when they don't actually care and just want working builds.

And even in the leave-as-is case, I still read the default that way. It makes no sense that below a certain version most people just don't want shared libraries. It makes far more sense that most people always want shared libraries, but also have static libraries than none at all. This PR exactly formalizies that second generalization of people's desires with the new default.

@Ericson2314
Copy link
Member

Oh, a change we could make is flip the or false to or true in case people forget the passthru, or just remove it altogether to require people to so-annotate their custom compilers.

I do see how a missing attribute causing static builds is a subtle and not-declarative-in-spirit default.

@peti
Copy link
Member

peti commented Jun 22, 2018

I just would have liked to have this conversation before the merge, not after. Anyway, I don't feel strongly enough about this issue to bother reverting those changes now. Both schemes have their advantages, and the state we have now feels just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: haskell 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants