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

Support gRPC 1.46.3 (also fix builds on Darwin) #138

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

unclechu
Copy link

@unclechu unclechu commented Jul 29, 2022

Closes #136.

Some context

I had to fix issues with building nixpkgs.grpc on Darwin using release-22.05 nixpkgs branch. The pin from gRPC-haskell (1.34.1) was giving some “undefined reference” errors coming from nixpkgs.abseil-cpp library as it seems. It turned out that nixpkgs.grpc from release-22.05 branch (1.46.3) builds just fine on Darwin but it’s not compatible with gRPC-haskell, there are some breaking changes in gRPC API. I decided it would be easier to just implement support for gRPC-haskell for 1.46.3 gRPC version than debugging the old 1.34.1 against Darwin, especially taking into account that I don’t have any Apple hardware by hand.

What I changed

The change to the API that makes it work with gRPC 1.46.3 is relatively small. There are few differences in the newer version of gRPC how the insecure/unauthenticated server and channel are created. This are no *_insecure_* functions anymore. Instead there are functions that are creating insecure credentials object.

Though I significantly refactored Nix configuration for the project:

  1. Updated nixpkgs pin to use the release-22.05 branch
  2. Added shell attribute to default.nix that I used to make this change, original shell.nix was failing to build for me when I was making a fix. After the fix it started to build fine, but while API was broken is was impossible to enter a nix-shell. So this addition based on haskellPackages.shellFor can be useful comparing to shell.nix which is just grpc-haskell.env.
  3. Added stack-env attribute to default.nix that makes Stack project buildable on NixOS (gmp and zlib are required). I used this command to build the project stack build --nix --system-ghc --no-test
  4. Created new pins for gRPC and its Python dependencies to match the same version as recommended in the comments of gRPC derivation
  5. Also created pins for the overridden Haskell dependencies so that the used patches are always applicable against other nixpkgs pins where the versions of those dependencies may change
  6. Updated proto3-wire and proto3-suite to match the versions from release-22.05 branch of nixpkgs
  7. Exposed in the attributes of default.nix overridden Haskell dependencies
  8. Used nixpkgs.python3 instead of nixpkgs.python which is Python 2 by default because some of the Python dependencies of gRPC do not support Python 2 anymore and entering nix-shell fails due to this
  9. Other changes are minor, more like stylistic ones

Note that I also updated the Stack configuration to use the newer snapshot to match the GHC version from nixpkgs pin. I also added stack.yaml.lock file to the project.

Open questions for the maintainer(s)

About tests

  • Just one test of grpc-haskell-core about graceful shutdowns (see LowLevelTests.testGoaway/core/tests/LowLevelTests.hs). But what’s weird that it’s not always failing. I managed to build it on one of my machines when it was successful. Do you have any ideas how to fix it or what could be the problem? It doesn’t seem like it’s related to my changes but rather to the newer version of gRPC for instance.
  • Attempt to build nix-build -A grpc-haskell without no-tests fails with: Builder called die: Cannot wrap 'tests/protoc.sh' because it is not an executable file. Is that a known issue?
  • What is the right way to run this example cabal configure --enable-tests && cabal test in a Nix Shell using new Cabal 3.x for instance? This command doesn’t work.
    $ cabal --version
    cabal-install version 3.6.2.0
    compiled using version 3.6.3.0 of the Cabal library
    $ cabal configure --enable-tests && cabal build && cabal test
    'cabal.project.local' already exists, backing it up to
    'cabal.project.local~0'.
    Warning: The package list for 'hackage.haskell.org' does not exist. Run 'cabal
    update' to download it.
    Resolving dependencies...
    cabal: Could not resolve dependencies:
    [__0] trying: grpc-haskell-0.3.0 (user goal)
    [__1] rejecting: grpc-haskell:!test (constraint from config file, command line
    flag, or user target requires opposite flag selection)
    [__1] trying: grpc-haskell:*test
    [__2] unknown package: tasty-hunit (dependency of grpc-haskell *test)
    [__2] fail (backjumping, conflict set: grpc-haskell, tasty-hunit,
    grpc-haskell:test)
    After searching the rest of the dependency tree exhaustively, these were the
    goals I've had most trouble fulfilling: grpc-haskell, grpc-haskell:test,
    tasty-hunit
    UPD: Just added missing pipes and tasty-hunit packages to the ghcWithPackages of grpc-haskell and it worked. Kind of. It runs the tests infinitely (never finishes):
    In order, the following will be built (use -v for more details):
     - grpc-haskell-0.3.0 (test:tests) (ephemeral targets)
    Preprocessing test suite 'tests' for grpc-haskell-0.3.0..
    Building test suite 'tests' for grpc-haskell-0.3.0..
    Running 1 test suites...
    Test suite tests: RUNNING...
    

Support of different gRPC versions

  • I think it would be better to support the old gRPC version as well, right? So maybe use CPP extension and add some conditions for the code. But the problem is that gRPC is not a Haskell package, there is no way to easily check the version of that C-library dependency version right? Maybe some Template Haskell magic can do that? Or maybe let’s provide gRPC version as Cabal flag? And default it to the supported 1.46.3 and in Nix there can be automatic set of this flag to the nixpkgs.grpc.version. What do you think about this?

Copy link
Contributor

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

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

I'm guessing that the reason for the unexpected failure is that the error code changed with the gRPC upgrade and you might have to add another case to the branch

@@ -0,0 +1,108 @@
# Copy-paste from nixpkgs “release-22.05” branch
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to pin grpc.nix if nixpkgs.nix now uses release-22.05?

Same question for the other grpc-related pins

Copy link
Author

Choose a reason for hiding this comment

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

@Gabriella439 technically no but the library is often used with different nixpkgs pins, not one provided by gRPC-haskell. So gRPC version can be different, thus there can be breaking changes. This feels more reliable to provide fixed pin of gRPC version this library specifically is built for.

release.nix Outdated Show resolved Hide resolved
inherit (pkgs) test-grpc-haskell;

inherit hsPkgsOverridden; # Function :: nixpkgs -> new haskellPackages
inherit (hsPkgsOverridden (nixpkgs {})) data-diverse proto3-wire proto3-suite;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to re-export data-diverse / proto3-wire / proto3-suite?

Copy link
Author

@unclechu unclechu Jul 30, 2022

Choose a reason for hiding this comment

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

@Gabriella439 if someone is using the library without using the overlay it can be really useful to reach those already patched dependencies (all these 3 require patching). When they are exported they are just ready to use.

@fumieval
Copy link
Contributor

fumieval commented Mar 7, 2023

Is there anything I can help to move this forward? @Gabriella439 @unclechu

@ixmatus
Copy link
Contributor

ixmatus commented May 5, 2023

@fumieval what would be helpful is if you could provide a review of this PR yourself, someone from our org will also have to (it will likely be me I think) but I'll have to swap in a lot so it would certainly help move things along if you provided a review...

rkaippully added a commit that referenced this pull request Jun 16, 2023
- Upgraded the pinned nixpkgs to nixos-21.11 which contains grpc-1.42.0. This requires no code changes to `grpc-haskell` and `grpc-haskell-core` libraries.
- Picked up latest commits of `proto3-{wire,suite}` and adjusted transitive dependencies
- Switched to python 3 because `grpcio-tools` requires it. Adjusted test scripts accordingly.
- Upgraded to latest CI action versions.

This change is minimal compared to #138 and will solve some test failures in that PR. Hopefully, this will make it easier to move that PR forward.
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.

Fails to build on Mac
4 participants