Skip to content

haskellPackages.readline: fix Setup.hs to work with Cabal 3#111985

Merged
cdepillabout merged 2 commits intoNixOS:release-20.09from
samuelrivas:fix-readline-for-cabal-3
Feb 9, 2021
Merged

haskellPackages.readline: fix Setup.hs to work with Cabal 3#111985
cdepillabout merged 2 commits intoNixOS:release-20.09from
samuelrivas:fix-readline-for-cabal-3

Conversation

@samuelrivas
Copy link
Copy Markdown
Contributor

Motivation for this change

Fix Haskell's readline package, which is marked as broken in the latest release.

Things done

Patched the Setup.hs file, to use Cabal 3's API, which is backwards incompatible with Cabal 2.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: haskell General-purpose, statically typed, purely functional programming language 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Feb 5, 2021

# Readline uses Distribution.Simple from Cabal 2, in a way that is not
# compatible with Cabal 3
readline = unmarkBroken (
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you're removed broken = true from hackage-packages.nix, you shouldn't need this unmarkBroken here.

Although you may want to remove readline from the list of broken packages in configuration-hackage2nix.yaml.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah true, I will update this

@@ -0,0 +1,8 @@
--- a/Setup.hs 2021-02-04 14:01:09.557970245 +0100
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general we try not to carry around patches in nixpkgs if it is possible to fetch them from upstream.

Can you fetch this from upstream?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Doesn't exist upstream as far as I am aware. The alternative would be to build this with Cabal 2 instead of Cabal 3. I thought it was using Cabal the build system, but it seems it is only using Cabal the library, so it might be possible to pin the dependency instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could create an issue upstream (or send a PR fixing this)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am planning to, but at the moment it is more urgent to me to get this fixed in nix, so I fixed it for myself with this change. If you prefer to wait until upstream fixes the problem that is fine for me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you want this fixed in Nix, I'd like you to be able to link to the upstream issue/PR so that it is obvious why the patch is needed (and we can keep track of whether or not we can ever remove the patch).

Also, if you send a PR upstream, you can sometimes pull the patch directly from that, so we don't have to carry it around in nixpkgs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think there is a problem upstream

Hmm, I'm not sure what could be going on here then. If this isn't a problem upstream, then it also shouldn't be a problem here in nixpkgs.

This started failing with a change in nixpkgs, I suspect that moving from Cabal 2 to Cabal 3, but I haven't verified it.

If this is failing upstream when building with Cabal 3, then it is arguably a problem upstream. I'd prefer that you send a proper fix upstream (or at least create an issue that the Nixpkgs maintainers can track), rather than just hacking around it here in nixpkgs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess we could say that the issue is that the package doesn't limit the version of Cabal here:

http://hackage.haskell.org/package/readline-1.0.3.0/src/readline.cabal

So a proper non invasive fix would be to edit the cabal file and state that Cabal version has to be lower than 3. Will nixpkgs build it with Cabal 2 if I changed that? I got the impression that the Cabal version is not configurable, but I may be wrong abou tthat.

I can't really find what upstream is for this package, I haven't seen any link to the source code other than the package uploaded to hackage.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, that is a tricky situation!

The email address in the cabal file is libraries@haskell.org, so I guess you could try to contact that mailing list, but eh, that's quite annoying to do.

In this case, how about we just go ahead and merge this in like you have it, since it doesn't sound like there is a very good solution.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

At least I can leave a comment in the nix config file, if that helps.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or, let me shoot them an email. I'll get back to you with the outcome of that.

@cdepillabout cdepillabout changed the title readline: fix Setup.hs to work with Cabal 3 haskellPackages.readline: fix Setup.hs to work with Cabal 3 Feb 9, 2021
@cdepillabout
Copy link
Copy Markdown
Member

@GrahamcOfBorg build haskellPackages.readline

@samuelrivas I updated the comment a little bit and pushed to your branch. I'll merge this in when CI has finished.

@cdepillabout
Copy link
Copy Markdown
Member

Ofborg passes, this looks good.

Thanks for working through this with me @samuelrivas!

@cdepillabout cdepillabout merged commit 88c0089 into NixOS:release-20.09 Feb 9, 2021
@samuelrivas
Copy link
Copy Markdown
Contributor Author

Awesome, thanks for your help. I'll get back to this when I get a response

@samuelrivas
Copy link
Copy Markdown
Contributor Author

I tried to reach libraries@haskell.org and my email bounced

You are not allowed to post to this mailing list, and your message has
been automatically rejected.  If you think that your messages are
being rejected in error, contact the mailing list owner at
libraries-owner@haskell.org.

I emailed the address suggested there too, but I haven't gotten any response. It seems this library is abandonware...

samuelrivas added a commit to samuelrivas/nixpkgs that referenced this pull request Aug 14, 2021
sternenseemann pushed a commit that referenced this pull request Aug 16, 2021
github-actions bot pushed a commit that referenced this pull request Aug 16, 2021
See #111985 for previous discussion

(cherry picked from commit b950eca)
sternenseemann pushed a commit that referenced this pull request Aug 16, 2021
See #111985 for previous discussion

(cherry picked from commit b950eca)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: haskell General-purpose, statically typed, purely functional programming language 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants