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

Update haskell packages to launch nix-shell in CONTRIBUTING.md #10221

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

taimoorzaeem
Copy link
Contributor

Running the given command in CONTRIBUTING.md, raises the following errors:

$ nix-shell -p cabal-install ghc ghcid haskellPackages.fourmolu_0_12_0_0 pkgconfig zlib.dev
     error: attribute 'fourmolu_0_12_0_0' missing
     at «string»:1:137:
          1| {...}@args: with import <nixpkgs> args; (pkgs.runCommandCC or pkgs.runCommand) "shell" { buildInputs = [ (cabal-install) (ghc) (ghcid) (haskellPackages.fourmolu_0_12_0_0) (pkgconfig) (zlib.dev) ]; } ""
           |                                                                                                                                         ^
     Did you mean one of fourmolu_0_15_0_0 or fourmolu_0_16_2_0?
   
     error: 'pkgconfig' has been renamed to/replaced by 'pkg-config'

This seemed like a simple fix which is updating these packages with the new package names. Hence the PR. Please review and merge.

@ulysses4ever
Copy link
Collaborator

The pkg-config update is fine but fourmolu is not: our codebase depends on 0.12 and is known to be not well-formed w.r.t. 0.15. So, for a proper fix, you probably need to learn how to get an arbitrary version of a package from Hackage. I couldn't quickly google how to do that but I seem to remember that there was such a thing.

@taimoorzaeem
Copy link
Contributor Author

Sorry, I couldn't help but think how would getting an arbitrary package from hackage is going to fix this? Could you elaborate further?

@geekosaur
Copy link
Collaborator

The Fourmolu versions in Nix won't work, as @ulysses4ever told you. You must install the older one somehow.

@taimoorzaeem
Copy link
Contributor Author

@ulysses4ever What do you think about adding a shell.nix file to the project. This way we can specify that we want to install the 0_12_0_0 version as shown in an example here here?

@ulysses4ever
Copy link
Collaborator

There are varying opinions among the active Cabal maintainers towards how much nix-related stuff could be added to the Cabal repository. I'm (apparently) in minority supporting some lightweight nix instrumentation but provided there's a smoke test for that in our CI. Most of the devs, though, seem to reject anything Nix-related in the repo, see this thread for more context #8753 (I'm honestly stunned that any nix stuff was allowed in the CONTRIBUTING.md at all)

@geekosaur
Copy link
Collaborator

geekosaur commented Jul 23, 2024

I think there's a difference between "here's some tips from Nix users on how to contribute if you use Nix", and providing (hence implying support, when we have none because none of us use Nix) Nix-related resources in our build tree (shell.nix, flake.nix, etc.).

xmonad has some Nix-related things in the tree, but they were accepted with the understanding that someone must support them or they will be jettisoned.

@ulysses4ever
Copy link
Collaborator

none of us use Nix

I maintain three local machines and one server that run NixOS. So, I very much use nix. I don't use nix for cabal development. Adding a shell.nix as suggested above would make my life a little easier (but not much). In return, I'm happy to oversee the proposed CI job. For something simple, along the lines of the SO referenced above, it will require very low maintenance, I think. But I agree that it's a different level of support requiring some discussion.

@ulysses4ever
Copy link
Collaborator

ulysses4ever commented Jul 24, 2024

The simplest way forward for this particular issue may be to remove fourmolu from the -p altogether and maybe add a note that "these are not a complete list of dependencies, e.g. fourmolu-0.12.0.0 used by the Cabal project is not available in nixpkgs-unstable". I'm also not sure why pkg-config is there tbh (maybe when you don't have it, some tests won't be run?..).

We could also reference https://github.com/yvan-sraka/cabal.nix but it looks a little lonely (updated 9 months ago).
EDIT: this repo seems to be doing the same mistake with haskellPackages.fourmolu_0_12_0_0 and not pinning nixpkgs to a point when fourmolu-0.12.0.0 was still there. Unless, that is, you're using it via the flake.

@taimoorzaeem taimoorzaeem force-pushed the docs/contrib-update branch 2 times, most recently from 6b6a509 to 859bb62 Compare July 27, 2024 04:58
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Makes sense to me!

@Kleidukos
Copy link
Member

@yvan-sraka Anything you can do on your cabal.nix setup?

CONTRIBUTING.md Outdated Show resolved Hide resolved
@taimoorzaeem taimoorzaeem force-pushed the docs/contrib-update branch 2 times, most recently from 90c12e5 to 3f90a22 Compare August 6, 2024 03:09
@ulysses4ever
Copy link
Collaborator

@fgaz @Kleidukos you approved the PR that added the initial version of these nix-shell-related instructions for cabal development. Would you consider giving a review to this update for the instructions?

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

Not that I'm qualified to review Nix stuff.

@ulysses4ever ulysses4ever added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Aug 6, 2024
@ulysses4ever
Copy link
Collaborator

I took the liberty to apply the label. If no updates come in two days, this will be auto-merged by the bot.

@ulysses4ever
Copy link
Collaborator

And thanks Brandon!

CONTRIBUTING.md Outdated
> ```
> One dependency that we left out in the above command is `haskellPackages.formolu_0_12_0_0` which would need to be installed manually.
Copy link
Member

Choose a reason for hiding this comment

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

We use fourmolu 0.14.0.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so:

version: "0.12.0.0"

Copy link
Member

Choose a reason for hiding this comment

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

Well-spotted, I was under a false impression.

Copy link
Member

Choose a reason for hiding this comment

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

@taimoorzaeem nevermind then, apologies for the confusion!

@taimoorzaeem taimoorzaeem force-pushed the docs/contrib-update branch 2 times, most recently from 7a435f4 to d02cf74 Compare August 6, 2024 15:08
@taimoorzaeem taimoorzaeem force-pushed the docs/contrib-update branch 2 times, most recently from d778a07 to 0cdec7e Compare August 6, 2024 15:10
@Kleidukos Kleidukos dismissed their stale review August 6, 2024 15:33

We still run fourmolu 0.12

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 8, 2024
@mergify mergify bot merged commit e8c35d8 into haskell:master Aug 8, 2024
13 checks passed
@taimoorzaeem taimoorzaeem deleted the docs/contrib-update branch August 12, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants