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

Allow bytestring-0.11 in test #88

Merged

Conversation

TristanCacqueray
Copy link
Contributor

This change enables running proto3-wire test with ghc-9.2.

This change enables running proto3-wire test with ghc-9.2.
@TristanCacqueray
Copy link
Contributor Author

The test compile, but it somehow still fails with:

tests:
<interactive>:1:1: warning: [-Wdeprecations]
    Module ‘Proto3.Wire.Builder’ is deprecated:
      This module is no longer used by the rest of the proto3-wire package.

Test suite tests: FAIL
Test suite logged to: dist/test/proto3-wire-1.4.0-tests.log
0 of 1 test suites (0 of 1 test cases) passed.

@j6carey
Copy link
Collaborator

j6carey commented Aug 8, 2022

The test compile, but it somehow still fails with:
...

The bit about deprecation is probably not relevant. How are you setting up your build environment with the new version of bytestring, and presumably GHC 9.2.x?

evanrelf
evanrelf previously approved these changes Aug 8, 2022
Copy link
Contributor

@evanrelf evanrelf left a comment

Choose a reason for hiding this comment

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

Tests pass in CI 👍

@j6carey
Copy link
Collaborator

j6carey commented Aug 8, 2022

I am guessing that bytestring-0.11 is key to triggering the problem, and our CI tests do not use it. In particular, this excellent change is likely the reason for the failure:

haskell/bytestring#175

I will have to update the low-level hackery to cope with the change, once I can get a build environment.

@TristanCacqueray
Copy link
Contributor Author

@j6carey oh well, trying to create a reproducer I see the tests are now passing, and the warning was a red herring :)

@TristanCacqueray
Copy link
Contributor Author

For what its worth, here is a nix-shell that show this patch working with ghc-9.2.4:

let
  nixpkgs = import (builtins.fetchTarball {
    url =
      "https://github.com/NixOS/nixpkgs/archive/00d73d5385b63e868bd11282fb775f6fe4921fb5.tar.gz";
  });
  pkgs = nixpkgs { };
  hspkgs = pkgs.haskell.packages.ghc924.extend (hpFinal: hpPrev: {
    data-diverse = pkgs.haskell.lib.dontCheck
      (pkgs.haskell.lib.overrideCabal hpPrev.data-diverse { broken = false; });
    proto3-wire = hpPrev.proto3-wire.overrideAttrs (_: {
      patches = [
        (pkgs.fetchpatch {
          url =
            "https://github.com/TristanCacqueray/proto3-wire/commit/14eff5d9b76e0f30b0735087e8a08edba610ea7a.patch";
          sha256 = "sha256-JwGGkrA9796btdsClaMjotBXPORCEkN+oB7Pr9GIMLc=";
        })
      ];
    });
  });

in hspkgs.ghcWithPackages (p: [ p.proto3-wire ])

@evanrelf
Copy link
Contributor

evanrelf commented Aug 8, 2022

I am guessing that bytestring-0.11 is key to triggering the problem, and our CI tests do not use it.

Oh yeah duh, I'm sorry 🙃 You're totally right.

Copy link
Collaborator

@j6carey j6carey left a comment

Choose a reason for hiding this comment

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

Ah, well, that's good new that your tests are passing, @TristanCacqueray . Maybe the pattern synonym for Data.ByteString.Internal.PS is saving us. If you are getting tests to pass, then this change is probably good enough.

@evanrelf evanrelf dismissed their stale review August 8, 2022 19:54

Incorrect review

@j6carey
Copy link
Collaborator

j6carey commented Aug 8, 2022

@TristanCacqueray , which branch of nixpkgs has 00d73d5385b63e868bd11282fb775f6fe4921fb5 ? (I ask because I'm getting an error when I try to build data-diverse in the latest master-branch nixpkgs.)

Answering my own question: haskell-updates.

@TristanCacqueray
Copy link
Contributor Author

@j6carey in NixOS/nixpkgs@00d73d5 you can see it is coming from a recent PR made to the haskell-updates branch

@j6carey
Copy link
Collaborator

j6carey commented Aug 8, 2022

@TristanCacqueray , I tried reproducing your results in a slightly different way: by making these changes to proto3-wire:

diff --git a/nix/nixpkgs.nix b/nix/nixpkgs.nix
index bb70009..cd5ecd0 100644
--- a/nix/nixpkgs.nix
+++ b/nix/nixpkgs.nix
@@ -2,8 +2,8 @@ args:
 
 let
   # nixpkgs release 21.05
-  rev = "7e9b0dff974c89e070da1ad85713ff3c20b0ca97";
-  sha256 = "1ckzhh24mgz6jd1xhfgx0i9mijk6xjqxwsshnvq789xsavrmsc36";
+  rev = "00d73d5385b63e868bd11282fb775f6fe4921fb5";
+  sha256 = "1hlhfgh3v6jvn125ck43i6sv6flwwv8vmk7cz5yrdcyh8lm86v6a";
 
   nixpkgs = builtins.fetchTarball {
     url = "https://github.com/NixOS/nixpkgs/archive/${rev}.tar.gz";
diff --git a/nix/pkgs.nix b/nix/pkgs.nix
index abe9410..9b5a56b 100644
--- a/nix/pkgs.nix
+++ b/nix/pkgs.nix
@@ -1,7 +1,7 @@
 import ./nixpkgs.nix {
   overlays = [
     (import ./haskell-packages.nix {
-      compiler = "ghc884";
+      compiler = "ghc924";
     })
   ];
 }

The result is this compilation error in a dependency:

test/Data/Diverse/ManySpec.hs:308:24: error:
    parse error on input ‘Bool’
    |
308 |             amend' @ '[Bool, Char] x (True ./ 'B' ./ nil) `shouldBe`
    |                        ^^^^

I'm still trying to figure out why...

@j6carey
Copy link
Collaborator

j6carey commented Aug 8, 2022

Looks like this is why: louispan/data-diverse@c721390

@j6carey
Copy link
Collaborator

j6carey commented Aug 8, 2022

I'm still having trouble getting the appropriate build environment; however, I see no advantage to restricting the version range in tests more than it is restricted in the library itself, so I think it is best to go ahead with the merge and unblock the general effort to support GHC 9.2 in nixpkgs.

@j6carey j6carey merged commit af71a6a into awakesecurity:master Aug 8, 2022
@TristanCacqueray
Copy link
Contributor Author

Thank you for the prompt feedback!

@j6carey
Copy link
Collaborator

j6carey commented Aug 8, 2022

@TristanCacqueray , you are welcome!

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.

3 participants