-
Notifications
You must be signed in to change notification settings - Fork 1
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 build #45
Update build #45
Conversation
This commit updates the build to pass on the latest github runners. To this end we: * update the github workflows to use the latest versions of all actions * enable the schnorr signature and the recovery module in the installation of the `secp256k1` library on all platforms to resolve any issues with missing symbols * update the stackage resolver to version lts-22.6 - the macOS runner has issues building on GHC 9.0.X * did not upgrade but added lower bounds to the `secp256k1-haskell` dependency of `< 1` as presumably we plan to move to the new `libsecp256k1` library * small changes `Bitcoin.Transaction.Builder` and `Bitcoin.Util` to make the code compile with the latest version of `bytestring` and remove the reliance on a missing (likely orphan) instances of `Alternative` for `Either a`. Signed-off-by: Tochi Obudulu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for the rapid response!
@@ -75,14 +75,15 @@ jobs: | |||
|
|||
- name: Install buildtools (MacOS) | |||
if: matrix.os == 'macOS-latest' | |||
run: brew install automake | |||
run: | | |||
brew install automake libtool | |||
|
|||
- name: Install libsecp256k1 (Unix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we want to handle this with nix? tbh I'm not actually a nix expert but afaict that's the way it's handled here. Not blocking but wanted to solicit some feedback here as it could alleviate you from having to do the cmake incantations below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of using nix here, although I believe that nix doesn't support windows yet without something like WSL. Having the nix configs in the stack.yaml might make it harder for contributor without a nix installation to build. If we're happy to, I can add it to the stack.yaml or we can define a separate one with the nix config as pass the file as an argument to all the stack commands in the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, and enough for me to put on hold any changes that move us towards nix. We can revisit in the future if someone is championing it.
This commit updates the build to pass on the latest github runners. To this end we:
secp256k1
library on all platforms to resolve any issues with missing symbolssecp256k1-haskell
dependency of< 1
as presumably we plan to move to the newlibsecp256k1
libraryBitcoin.Transaction.Builder
andBitcoin.Util
to make the code compile with the latest version ofbytestring
and remove the reliance on a missing (likely orphan) instances ofAlternative
forEither a
.