Skip to content

miso: init at 0.2.0.0 #27322

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
Merged
merged 1 commit into from
Jul 13, 2017
Merged

miso: init at 0.2.0.0 #27322

merged 1 commit into from
Jul 13, 2017
+11 −0

Conversation

dmjio
Copy link
Member

@dmjio dmjio commented Jul 12, 2017 •
edited
Loading

Adds miso-0.2.0.0 to ghcjs overrides
cc @peti

Motivation for this change

To add the correct dependencies to the ghcjs version of miso.

Currently, the version of miso in hackage-packages.nix is for ghc only (i.e. doesn't have correct ghcjs depedencies -- since it wasn't generated with cabal2nix appropriately. It needs to specify the correct compiler flag, cabal2nix --compiler ghcjs, otherwise it will not build). This override specifies a new derivation with the correct client dependencies when using haskell.packages.ghcjs.miso (basically same as calling cabal2nix --compiler ghcjs)

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@mention-bot
Copy link

@dmjio, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cstrahan, @Profpatsch and @k0001 to be potential reviewers.

peti
peti requested changes Jul 12, 2017
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.

Please add a comment to explain why that override is necessary and what problem it fixes.

@dmjio dmjio force-pushed the patch-4 branch 3 times, most recently from a7c5458 to 1b3b01a Compare July 12, 2017 14:39
@dmjio
Copy link
Member Author

dmjio commented Jul 12, 2017 •
edited
Loading

@peti thanks for review, I forgot to include the entire derivation with correct dependencies. I then also forgot to include the final {} in the call to callPackage.... :)

@dmjio
Copy link
Member Author

dmjio commented Jul 12, 2017 •
edited
Loading

It seems to be working on nixos (w/ useSandbox)

nix-repl> :b haskell.packages.ghcjs.miso
warning: you did not specify ‘--add-root’; the result might be removed by the garbage collector
/nix/store/1p3p5yc4chkmbmxylb8djq9jg3dmqsp7-miso-0.2.0.0

this derivation produced the following outputs:
  out -> /nix/store/1p3p5yc4chkmbmxylb8djq9jg3dmqsp7-miso-0.2.0.0

peti
peti requested changes Jul 12, 2017
View reviewed changes
Copy link
Member

@peti peti left a comment •
edited
Loading

Choose a reason for hiding this comment

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

IMHO, it would be much better to augment the existing expression of miso with the missing settings, i.e. via overrideCabal. Replacing the whole expression feels like overkill, particularly the override of the src field, which means that this override effectively prevents any version updates on Hackage from making it into the package set.

Also, please add a comment to the code that explains why this override is necessary in the first place.

@dmjio
Copy link
Member Author

dmjio commented Jul 12, 2017 via email

@dmjio dmjio force-pushed the patch-4 branch 2 times, most recently from 4fb40c3 to 7288801 Compare July 12, 2017 16:28
@dmjio
Copy link
Member Author

dmjio commented Jul 12, 2017

@peti, have made more changes in response to the review, please advise.

@peti
Copy link
Member

peti commented Jul 12, 2017 •
edited
Loading

Overriding is necessary because the derivation on hackage-packages.nix was not created with --compiler ghcjs.

Yes, I understand that. But other people might not, and it would be great if you could document the purpose of the override in the source code so could that casual readers who look at that file can see what is going on without having to resort to git annotate and whatnot.

@dmjio
miso: init at 0.2.0.0

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
51edf3b
@dmjio
Copy link
Member Author

dmjio commented Jul 12, 2017 •
edited
Loading

cc @peti, documentation added

@peti peti merged commit 4a599a6 into NixOS:master Jul 13, 2017
@dmjio
Copy link
Member Author

dmjio commented Jul 13, 2017

@peti 🍻

@dmjio dmjio deleted the patch-4 branch July 13, 2017 10:40
@peti
Copy link
Member

peti commented Jul 13, 2017

Thank you, @dmjio, for submitting that fix!

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

@peti peti

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dmjio @mention-bot @peti