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

[haskell/generic-builder] windows always has an active library #43712

Merged
merged 2 commits into from Jul 27, 2018
Merged

[haskell/generic-builder] windows always has an active library #43712

merged 2 commits into from Jul 27, 2018

Conversation

angerman
Copy link
Contributor

Motivation for this change

Broken out of #43559

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@angerman
[haskell/generic-builder] windows always has an active library
5160290 
This commit is not really correct. The `hasActiveLibrary` check is wrong.
We can have an active library even if we do not ask for a static lirbary or
dynamic one; we can still have just a set of objet files and archives.
@angerman angerman requested a review from peti as a code owner July 18, 2018 02:36
@peti
Copy link
Member

peti commented Jul 18, 2018

I am sorry, but I don't understand the reason behind this change. I also read the commit message and now I am even more confused than before. Could you please re-state your reasons for making this change to help me understand what you mean by "the check is wrong"? And why is Windows different than other platforms?

@angerman
Copy link
Contributor Author

@peti, fair point, this is a rather confusing PR. And I'm not even happy with it.

Here's the underlying issue: nixpkgs assumes that there is a library if it's either dynamic, or static, or profiling; and it's a library to begin with. This however is not correct. You can have a library that is neither --enable-static, nor --enable-shared. That is you end up with a library that is just a collection of obejct files as well as an aggregated archive.

The underlying issue is that --enable-static with cabal passes -staticlib to ghc, and -staticlib does not work on windows, and as such you can't set enableStaticLibraries, but at the same time if you don't want enableSharedLibraries, you end up with empty libraries, which is certainly not what you want.

@peti peti removed their request for review July 21, 2018 07:37
peti
peti reviewed Jul 21, 2018
View reviewed changes
Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just trust your judgement going forward with this. Please do whatever you think is best. :-)

matthewbauer
matthewbauer approved these changes Jul 23, 2018
View reviewed changes
Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an okay thing for now. Especially if it gets us closer to ghc/mingw support.

@angerman
Copy link
Contributor Author

To be fair, I think the correct solution is to get rid of the hasActiveLibrary logic, and just go with isLibrary.
The reasoning is as follows:

  • enableStaticLibraries -> hasActiveLibrary
  • enableSharedLibraries -> hasActiveLibrary
  • enableLibraryProfiling -> hasActiveLibrary
    and as I outlined above, !enableStaticLibraries && !enableSharedLibraries && !enableLibraryProfiling should not imply !hasActiveLibrary.

@domenkozar
Copy link
Member

target haskell-updates branch

@angerman
Update generic-builder.nix
7d97fd3 
Drop `hasActiveLibrary` altogether. The condition is wrong, `isLibrary` is the correct one. We can have non-static, non-shared libraries as well.
@angerman angerman changed the base branch from master to haskell-updates July 27, 2018 04:25
@angerman
Copy link
Contributor Author

@domenkozar alright :-) haskell-updates it is.

@ryantm ryantm merged commit bf5710c into NixOS:haskell-updates Jul 27, 2018
@ryantm
Copy link
Member

ryantm commented Jul 27, 2018

I didn't look at this carefully, but I'm going to merge it based on all the approval it has. Let's see how it does on Hydra!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peti peti peti left review comments

@matthewbauer matthewbauer matthewbauer approved these changes

@basvandijk basvandijk Awaiting requested review from basvandijk

@ryantm ryantm Awaiting requested review from ryantm

Assignees
No one assigned
Labels
6.topic: haskell 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
@angerman @peti @domenkozar @ryantm @matthewbauer @GrahamcOfBorg