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

Add hack for common erroneous missing build-tool-depends, and keep duplicate deps for cross #358

Merged
merged 3 commits into from Jul 12, 2018

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jul 11, 2018

See comment in code for more details.

The cross change is unambiguously a good thing, and put in the first commit. The *-discover hack is more a stop-gap until packages stop being broken---we're not hacking around a Nix deficiency with strictDeps here. It relies on the cross change because rather than changing what sort of dependency *-discover is, it adds a new dependency in case it was actually wanted as a library, which would be removed.

Fixes #356

CC @matthewbauer @domenkozar

@domenkozar
Copy link
Member

Also needs: hsx2hs, markdown-unlit.

@Ericson2314
Copy link
Member Author

@domenkozar OK will do.

Any idea what's going on with CI? Seems like a preexisting issue.

@domenkozar
Copy link
Member

@Ericson2314 you're hitting #359

@Ericson2314
Copy link
Member Author

Thanks!

@peti peti force-pushed the master branch 2 times, most recently from 0cbb88c to 66799f0 Compare July 12, 2018 09:49
@Ericson2314
Copy link
Member Author

Ericson2314 commented Jul 12, 2018

@peti You seen this yet? I can fix that CI issue, but it's unrelated to this patch.

@peti
Copy link
Member

peti commented Jul 12, 2018

The CI problems are fixed now.

@Ericson2314
Copy link
Member Author

Thanks! rebased and pushed.

@domenkozar
Copy link
Member

src/Distribution/Nixpkgs/Haskell/FromCabal/PostProcess.hs:24:12: Warning: Redundant bracket
Found:
  (id)
Perhaps:
  id
src/Distribution/Nixpkgs/Haskell/FromCabal/PostProcess.hs:60:37: Warning: Redundant bracket
Found:
  (id)
Perhaps:
  id
2 hints

See comment in code for more details.

Fixes NixOS#356
@Ericson2314
Copy link
Member Author

Ericson2314 commented Jul 12, 2018

@domenkozar thanks for fishing that out. Fixed, and ran hlint locally too.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jul 12, 2018

OK cool it built. I guess next step is to revive up strictdeps patch with this while waiting for review? I see the script to regenerate nixpkgs, but not sure what the normal flow (e.g. push to hackage then use that version?) so I'll let @peti do that vs speculatively do in nixpkgs PR.

@peti peti merged commit 0f70a5b into NixOS:master Jul 12, 2018
@peti
Copy link
Member

peti commented Jul 12, 2018

The re-generated package set is in haskell-updates @ peti/nixpkgs@8e47462. Test builds are running at https://hydra.nixos.org/jobset/nixpkgs/haskell-updates.

@domenkozar
Copy link
Member

Now we need to include strictDeps change and reenable all packages since many should build now (mostly macos). @Ericson2314 are you going to re-do that PR?

@Ericson2314 Ericson2314 deleted the missing-tools branch July 13, 2018 13:33
@domenkozar
Copy link
Member

This still doesn't add hspec-discover for packages that don't declare it as dependency such as unliftio:

  "unliftio" = callPackage
    ({ mkDerivation, async, base, deepseq, directory, filepath, hspec
     , process, stm, time, transformers, unix, unliftio-core
     }:
     mkDerivation {
       pname = "unliftio";
       version = "0.2.7.0";
       sha256 = "0qql93lq5w7qghl454cc3s1i8v1jb4h08n82fqkw0kli4g3g9njs";
       libraryHaskellDepends = [
         async base deepseq directory filepath process stm time transformers
         unix unliftio-core
       ];
       testHaskellDepends = [
         async base deepseq directory filepath hspec process stm time
         transformers unix unliftio-core
       ];
       description = "The MonadUnliftIO typeclass for unlifting monads to IO (batteries included)";
       license = stdenv.lib.licenses.mit;
     }) {};

@Ericson2314
Copy link
Member Author

I think that's reasonable.

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

Successfully merging this pull request may close these issues.

None yet

3 participants