Skip to content

Fix setup-depends dependencies #124

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

Merged
merged 13 commits into from
Jul 12, 2019
Merged

Fix setup-depends dependencies #124

merged 13 commits into from
Jul 12, 2019

Conversation

hamishmack
Copy link
Collaborator

@hamishmack hamishmack commented May 14, 2019

@Ericson2314
Copy link

BTW it's been my long-standing intention to make setup a proper component in Cabal/cabal-install too.

@angerman
Copy link
Collaborator

@Ericson2314 please be my guest! ;-) is be happy to see the Setup.hs being a component. You’ll end up with some minor bootstrapping issue though. You’ll need the Setup.hs to build the Setup component :-) Unless you go through cabal-install, but then how do you build cabal-install?

@hamishmack hamishmack mentioned this pull request May 15, 2019
@Ericson2314
Copy link

@angerman setup components would be exe components without custom setups in practice, but yes the implementation would probably be able to deal with such a thing. It's not that different than setup-depends libraries having custom setups, which already works.

haskell/cabal#4047 was an issue made after I talked to ezyang. Wow that was a long time ago. I think after I'm with my current rummaging around in GHC I'll get back to Cabal so will still be a long ways off.

@hamishmack hamishmack force-pushed the hkm/fix-build-depends branch 2 times, most recently from 3944a13 to df8c734 Compare May 29, 2019 05:41
@hamishmack
Copy link
Collaborator Author

This PR had conflicts with the shellFor stuff that has now been merged into master. Mostly because the setup builder was moved into hspkg-builder.nix (from default.nix). I have rebased this PR so it now modifies hspkg-builder.nix.

@rvl
Copy link
Contributor

rvl commented May 29, 2019

Sorry about that, I had to move stuff.

@hamishmack
Copy link
Collaborator Author

Sorry about that, I had to move stuff.

No worries. Totally worth it, the shellFor stuff is awesome.

@angerman
Copy link
Collaborator

angerman commented Jun 8, 2019

@hamishmack can we rebase this and get it ready for merging?

@angerman
Copy link
Collaborator

@hamishmack is this still a draft or is this ready?

@hamishmack hamishmack marked this pull request as ready for review June 16, 2019 09:26
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Apologies for the late review.

Would it be possible to use the makeConfigFiles function in setup-builder.nix?

Because I see a chunk of hairy code which is duplicated here.

@rvl rvl force-pushed the hkm/fix-build-depends branch from fac642f to cd1f96f Compare June 18, 2019 12:24
@rvl
Copy link
Contributor

rvl commented Jun 18, 2019

It looks better now. But I can still see make-config-files is imported twice. Would it be possible to define all the parts of the builder at the top-level builder/default.nix? This will untangle the dependencies a bit. I pushed a commit with this rearrangement, but didn't test that it works.

@angerman
Copy link
Collaborator

angerman commented Jul 1, 2019

@hamishmack can we get this finished?

@hamishmack hamishmack force-pushed the hkm/fix-build-depends branch from ebd61c7 to 95f1ae7 Compare July 2, 2019 09:11
@hamishmack hamishmack force-pushed the hkm/fix-build-depends branch from 83735ba to 3872708 Compare July 2, 2019 09:36
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@hamishmack
Copy link
Collaborator Author

The test failure was related to cabal-doctest not being in the pkgs.nix for nix-tools. This is likely to be a common problem. The root cause is that plan.json output does not include the packages referenced in the setup-depends of packages it plans to get from hackage. In this case the distributive package is a needed by nix-tools and it wants cabal-doctest for its setup depends.

@rvl
Copy link
Contributor

rvl commented Jul 3, 2019

The test failure was related to cabal-doctest not being in the pkgs.nix for nix-tools.

Hmm, tricky. Rather than fiddling with the project package set, could we grab the setup depends out of a separate package set? (Like the haskellPackages top-level attribute of Haskell.nix)

@hamishmack hamishmack force-pushed the hkm/fix-build-depends branch from 7dbe956 to a1fd9de Compare July 4, 2019 00:35
@hamishmack
Copy link
Collaborator Author

It turns out that yesterday when I tested plan-to-nix I was inside a shell that had distributive in ghc-pkg list. As a result its setup depends (like cabal-doctest) were not included in plan.json.

I tried again with just ghc 8.6.5 in the shell and cabal new-configure included cabal-doctest in plan.json. I ran plan-to-nix and have committed the results to this PR.

I think we should be ok as long as people use callCabalProjectToNix or take care not to run plan-to-nix inside a shellFor.

@hamishmack
Copy link
Collaborator Author

take care not to run plan-to-nix inside a shellFor

I should have said: take care not to use plan-to-nix on a plan.json file generated inside a shellFor.

The file `conduit.hs` looks like `Conduit.hs` to ghc and it uses it
by mistake for `import Conduit`.
@rvl
Copy link
Contributor

rvl commented Jul 5, 2019

take care not to use plan-to-nix on a plan.json file generated inside a shellFor.

That's interesting. Maybe I should I document it? Or maybe there's some other way of avoiding this problem. Seems like something that users might hit every so often.

@ocharles
Copy link
Contributor

ocharles commented Jul 5, 2019

I would note I made exactly the same mistake. We use the normal nixpkgs Haskell stuff and have a working shell, so when I ran plan-to-nix I ended up with something entirely wrong. It was only when I ran it in a pure shell that I got the correct plan. It's worth documenting this (though ideally one would use callCabalProjectToNix which avoids this problem, at the cost of IFD).

@hamishmack hamishmack force-pushed the hkm/fix-build-depends branch from bfaa6d8 to b224850 Compare July 9, 2019 03:01
@hamishmack hamishmack force-pushed the hkm/fix-build-depends branch from 6637811 to 0975269 Compare July 10, 2019 03:23
@rvl
Copy link
Contributor

rvl commented Jul 12, 2019

Just checking, are the latest commits necessary to fix setup-depends dependencies?

Could we merge a basic fix first, then address enabling tests and more cross-compilation fixes?

@hamishmack
Copy link
Collaborator Author

I am not sure they are necessary. I am worried that the setup-depends changes introduce new dependencies on buildPackages and may introduce regressions in cross compilation.

@hamishmack hamishmack requested review from rvl and angerman July 12, 2019 09:03
@angerman
Copy link
Collaborator

I agree with @rvl here. I think it would be good to have this one split, with the last two commits in a separate PR. We are going to squash this anyway, and I'm afraid we are conflating stuff here otherwise. Specifically as 8faff20 looks remarkably like #134. Also, CI was green at 2d2b4f9. So I think this should be fine to split.

@hamishmack hamishmack force-pushed the hkm/fix-build-depends branch from 8faff20 to 2d2b4f9 Compare July 12, 2019 13:12
@hamishmack
Copy link
Collaborator Author

Ok. I have removed the last two commits.

Copy link
Collaborator

@angerman angerman left a comment

Choose a reason for hiding this comment

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

One more change and I think we are golden :-)

@@ -0,0 +1,46 @@
{ stdenv, buildPackages, haskellLib, ghc, nonReinstallablePkgs, hsPkgs, makeConfigFiles }:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{ stdenv, buildPackages, haskellLib, ghc, nonReinstallablePkgs, hsPkgs, makeConfigFiles }:
{ buildPackages, stdenv, lib, haskellLib, ghc, nonReinstallablePkgs, hsPkgs, makeConfigFiles }:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems to be outdated.

Co-Authored-By: Moritz Angermann <[email protected]>
@angerman angerman merged commit efab350 into master Jul 12, 2019
@angerman angerman deleted the hkm/fix-build-depends branch July 12, 2019 13:48
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.

5 participants