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

Switch everything over to Cabal #116

Merged
merged 9 commits into from
Jun 19, 2023
Merged

Conversation

robbert-vdh
Copy link
Member

Just like in our other projects. I also took the opportunity to move the Nix builds overt to the more modern overlay style where the icepeak and icepeak-client packages are added to the Haskell packages as an overlay. They're then exposed in build.py so the CI can easily build both of them separately. I've made these changes:

  • Instead of treating this like two separate projects with their own stack.yaml, there's now a single default.nix file and the cabal.project file in the repository's root now references both packages. That means you can run cabal run <icepeak/icepeak-client/all> from anywhere in the repo to build/run the packages.
  • There's now a .envrc file for Nix integration with direnv, since Hoff and all our other repos also have this.
  • I haven't fully modernized the Seamaphore build, but I did remove the explicit stack step since it's redundant with the nix-build and that invokes Cabal anyways.
  • I wasn't quite sure what to do with the integration tests, which is why I marked this as a draft to avoid merging. As of right now, half of them fail. Looking at the git logs it looks like they haven't been touched for a couple years. @fatho may have been one of the last persons to work on them. I changed the shell script ones to invoke Cabal instead of Stack, but there are also a couple standalone Haskell files in there. Do we want to keep these? If so, they should probably be moved to a proper integration test binary instead of being standalone files.

There's now a single `default.nix` file that exposes the environment, so
the client library and s server application don't have their own
`default.nix`.

The build style has been moved to a Haskell overlay, and `release.nix`
exposes the two packages as attributes.
Hoff also has this, so why not do the same thing for icepeak.
This lets you easily enable or disable it through the command line or by
overriding it in `cabal.project.local`.
These .hs files are a bit hacky and would probably need to be reworked
to be usable again.
@FPtje FPtje requested review from HugoPeters1024 and removed request for FPtje June 14, 2023 10:02
@FPtje
Copy link

FPtje commented Jun 14, 2023

Asking @HugoPeters1024 to take a look, because I'm rather busy. @HugoPeters1024 Do you have time to look at this? Thanks!

@@ -1,6 +1,9 @@
#!/usr/bin/env stack
-- stack --stack-yaml ../../client-haskell/stack.yaml runghc --package QuickCheck --package quickcheck-text

-- FIXME: These scripts will either need to be dropped, or they should be moved

Choose a reason for hiding this comment

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

Do we want to postpone this? Altough I'm assuming this script is rarely used.

Copy link
Member Author

Choose a reason for hiding this comment

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

They probably haven't been used in years since they won't run correctly as is. @fatho was the last one who worked on this. We can probably also just leave them as is and fix them up once they're needed again.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this before but I now found the actual ticket that was closed with a #wontfix. I had already tried converting the scripts to cabal run scripts but that didn't work. This is why: haskell/cabal#8562

exit 1
fi
cd "$(dirname "$0")"
cd ..

Choose a reason for hiding this comment

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

Thanks I hate it (but can't think of why this is objectively bad)

Copy link
Member Author

Choose a reason for hiding this comment

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

cd "$(dirname "$(dirname "$0")")"

doesn't work if the script is invoked relatively from the same directory or through a couple other names. This works, and I've done it in the Hoff repo, but it's a bit uglier so just doing the two cds is probably better:

cd "$(dirname "$(dirname "$(realname "$0")")")"

@@ -8,47 +8,7 @@ agent:
os_image: "ubuntu2004"

blocks:
- name: "Run checks"

Choose a reason for hiding this comment

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

are we just getting rid of running tests in CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, they're still run. From the OP:

  • I haven't fully modernized the Seamaphore build, but I did remove the explicit stack step since it's redundant with the nix-build and that invokes Cabal anyways.

@@ -85,11 +45,11 @@ blocks:
jobs:
- name: "Build client"
commands:
- "nix-build --no-out-link -E '(import ./nix/nixpkgs.nix {}).haskellPackages.callPackage ./client-haskell/client.nix {}'"

Choose a reason for hiding this comment

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

could you also incorporate running cabal-fmt somewhere in the CI? (formatter for the cabal file)

Copy link
Member Author

Choose a reason for hiding this comment

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

In the other repos it's part of build.py but we don't have a build.py here. Or any other lints for that matter. The CI just checks if it builds and the tests pass. There are currently no lints or style checks at all.

Choose a reason for hiding this comment

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

I suppose the cabal file will almost never change in this project, LGTM

@robbert-vdh robbert-vdh marked this pull request as ready for review June 19, 2023 09:15
@robbert-vdh
Copy link
Member Author

Since they currently don't run cleanly anyways, we decided to just look into fixing up the stack runghc-based integration later if we ever need to run them again.

@OpsBotPrime merge

@OpsBotPrime
Copy link
Contributor

Pull request approved for merge by @robbert-vdh, rebasing now.

Approved-by: robbert-vdh
Auto-deploy: false
@OpsBotPrime
Copy link
Contributor

Rebased as 2109f59, waiting for CI …

@OpsBotPrime
Copy link
Contributor

CI job 🟡 started.

@OpsBotPrime OpsBotPrime merged commit 2109f59 into master Jun 19, 2023
@OpsBotPrime OpsBotPrime deleted the robbert-vdh/cabalification branch June 19, 2023 09:33
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.

4 participants