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.

Sign up for GitHub

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

Jump to bottom

echidna: 1.7.3 -> 2.0.2 #190144

Merged
merged 1 commit into from
Sep 9, 2022
Merged

echidna: 1.7.3 -> 2.0.2 #190144

merged 1 commit into from
Sep 9, 2022

Conversation

arcz
Copy link
Member

@arcz arcz commented Sep 7, 2022

Description of changes

This fixes echidna that suffered from breakage of hevm. We are working on updating hevm to move forward with dependencies but it will take some time. Until then, both hevm and echidna need little patches to update aeson that changed API with the major update.

I tried to revert aeson to the previous version but that started a rabbit hole of incompatibilities.

Fixes #182500.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@hellwolf
Copy link
Contributor

hellwolf commented Sep 8, 2022

what can we do to unblock this from being merged?

@arcz
Copy link
Member Author

arcz commented Sep 8, 2022

@IvarWithoutBones what you pointed out makes total sense, I've made those changes. I originally started with another approach and didn't realize things can be moved around.

cdepillabout
cdepillabout requested changes Sep 8, 2022
View reviewed changes
Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

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

Thanks for sending this PR.

Couple pieces of feedback:

  1. We generally ask all haskell-related PRs to go to the haskell-updates branch. Could you rebase on there and change the base branch here on GitHub?

And then some more inline.

pkgs/development/haskell-modules/configuration-nix.nix Outdated
@@ -739,7 +739,7 @@ self: super: builtins.intersectAttrs super {
}) super.haskell-language-server;

# tests depend on a specific version of solc
hevm = dontCheck (doJailbreak super.hevm);
hevm = unmarkBroken (dontCheck (appendPatch ./patches/hevm-update-deps.patch super.hevm));
Copy link
Member

Choose a reason for hiding this comment

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

  1. We almost never use unmarkBroken. Instead, you'll need to remove hevm from the broken list in pkgs/development/haskell-modules/configuration-hackage2nix/broken.yaml and run maintainers/scripts/haskell/regenerate-hackage-packages.sh.
  2. We try to never carry around .patch files in Nixpkgs because it bloats the repo. If at all possible, we like to pull commits or PRs from upstream with fetchpatch. There should be a bunch of examples of that in this file and pkgs/development/haskell-modules/configuration-common.nix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed 2.

pkgs/development/haskell-modules/patches/hevm-update-deps.patch
transformers >= 0.5.6 && < 0.6,
tree-view >= 0.5 && < 0.6,
abstract-par >= 0.3.3 && < 0.4,
- aeson >= 1.5.6 && < 1.6,
Copy link
Member

Choose a reason for hiding this comment

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

  1. Instead of patching these out, you could get away with just using doJailbreak. That should require less maintenance in the long-run (but be slightly harder to debug when it ends up no longer working).

pkgs/tools/security/echidna/default.nix
@@ -1,5 +1,4 @@
{ lib
Copy link
Member

Choose a reason for hiding this comment

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

  1. Is there some reason echidna is not on Hackage? Could you upload it to hackage? That way, you wouldn't have to maintain this file here, but it would get updated automatically with the rest of the Haskell packages in Nixpkgs.

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 have to speak with the rest of the echidna team, but I think we can do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it okay if we merge this and I'll work on hackage for the next update?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that sounds good, thanks!

@arcz arcz changed the base branch from master to haskell-updates September 8, 2022 19:56
@arcz
Copy link
Member Author

arcz commented Sep 8, 2022

@cdepillabout I rebased the PR. Regarding other comments, the situation with hevm is a bit problematic at the moment. I updated the dependencies in the upstream a while ago here dapphub/dapptools#938 but the integration test suite there had issues after that. On top of that, recently, the development of hevm moved to https://github.com/ethereum/hevm with the deps updated but I'm waiting for the 0.50.0 release as the code is in development state right now. I think it's best to just keep those patches until 0.50.0 is published and I'll clean it all up when it lands.

@cdepillabout
Copy link
Member

@arcz So there is currently no commit in any upstream that makes these same compatibility changes?

@arcz
Copy link
Member Author

arcz commented Sep 8, 2022

Right, I don't think there is a sensible commit to pick as a patch. I hand-crafted them.

@cdepillabout
Copy link
Member

@arcz Okay, in that case it sounds like we have no choice. Could you make sure to put a good comment on them where you pull them in describing why they are necessary and when we expect to be able to get rid of them?

@arcz
Copy link
Member Author

arcz commented Sep 8, 2022

@cdepillabout done

@cdepillabout cdepillabout merged commit 0f61ca9 into NixOS:haskell-updates Sep 9, 2022
@cdepillabout
Copy link
Member

LGTM, thanks for working through this one!

If you're interested in making sure echidna and hevm don't break in the future, feel free to add yourselves as maintainers. That way you'll get pinged if hackage updates ever cause echidna/hevm to stop compiling.

@arcz arcz mentioned this pull request Nov 16, 2022
Merged
@arcz arcz deleted the echidna-2.0.2 branch November 27, 2022 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvarWithoutBones IvarWithoutBones IvarWithoutBones left review comments

@cdepillabout cdepillabout cdepillabout requested changes

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

@maralorn maralorn Awaiting requested review from maralorn

Assignees
No one assigned
Labels
6.topic: haskell 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update echidna from 1.7.3 to 2.0.2
4 participants
@arcz @hellwolf @cdepillabout @IvarWithoutBones