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

haskell-updates fixes: hevm #303658

Merged
merged 1 commit into from
Jun 23, 2024
Merged

haskell-updates fixes: hevm #303658

merged 1 commit into from
Jun 23, 2024

Conversation

hellwolf
Copy link
Contributor

@hellwolf hellwolf commented Apr 12, 2024 •
edited
Loading

Description of changes

I am fully aware the ongoing work at ethereum/hevm#471. But in the interest of unblocking hevm and its downstream packages. I made this fix for now, and I will monitor the upstream work and remove the patch when needed.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@maralorn
Copy link
Member

maralorn commented Apr 12, 2024 •
edited
Loading

Thank you.

Can you follow the examples in configuration-common.nix and fetch this patch from GitHub with fetchpatch?

@hellwolf
Copy link
Contributor Author

Can you follow the examples in configuration-common.nix and fetch this patch from GitHub with fetchpatch?

Oh, at the moment, there is no such patch as permanent link from github available. I think I found other examples who store patch under pkgs/development/haskell-modules/patches/. What's the guideline here?

@maralorn
Copy link
Member

Can you follow the examples in configuration-common.nix and fetch this patch from GitHub with fetchpatch?

Oh, at the moment, there is no such patch as permanent link from github available. I think I found other examples who store patch under pkgs/development/haskell-modules/patches/. What's the guideline here?

I haven’t heard about a patch link which can’t be made into a permanent one. What’s the issue, do you need a commit range? GitHub can do that.

The guideline is: Avoid if sensibly possible.

@hellwolf
Copy link
Contributor Author

hellwolf commented Apr 12, 2024 •
edited
Loading

I haven’t heard about a patch link that can’t be made into a permanent one. What’s the issue? Do you need a commit range? GitHub can do that.

I didn't say it was not possible. I was just saying it was not there yet. I will create one in the right place. No problem

The guideline is: Avoid if sensibly possible.

@hellwolf hellwolf marked this pull request as ready for review June 22, 2024 14:04
@hellwolf hellwolf marked this pull request as draft June 22, 2024 14:04
@hellwolf hellwolf changed the title hevm: fix 0.5.3 build haskell-updates fixes: hevm Jun 22, 2024
@hellwolf hellwolf marked this pull request as ready for review June 22, 2024 20:34
@hellwolf hellwolf changed the title haskell-updates fixes: hevm haskell-updates fixes: hevm, hourglas and crypton-x509 Jun 22, 2024
@hellwolf hellwolf changed the title haskell-updates fixes: hevm, hourglas and crypton-x509 haskell-updates fixes: hevm, hourglass and crypton-x509 Jun 22, 2024
@hellwolf
Copy link
Contributor Author

Updated the fixes, and now ready for reviewing:

  1. hevm patch now is fetched from github
  2. hourglass needs jailbreaking.
  3. crypton-x509 no longer needs special configuration

CC @maralorn

@maralorn
Copy link
Member

We prefer not to merge the base branch into pull requests, our git history is long enough as it is.

@maralorn
Copy link
Member

Thank you!

I will do the hourglass fix manually, since we need it on the branch now.

The url to the hevm patch is unstable. Can you please change that to an explicit commit range? Also can you please add a comment to the upstream PR?

@maralorn
Copy link
Member

I settled on a different fix for hourglass so you can remove your override from this PR. (after a rebase)

@maralorn
Copy link
Member

Me again, sorry. I also applied the crypton-x509 fix on the branch, so you can also drop that. :D

@hellwolf
Copy link
Contributor Author

We prefer not to merge the base branch into pull requests, our git history is long enough as it is.

okay, I just rebased instead.

@hellwolf
Copy link
Contributor Author

I will do the hourglass fix manually, since we need it on the branch now.

Ack, I have removed my fix.

@hellwolf
Copy link
Contributor Author

Me again, sorry. I also applied the crypton-x509 fix on the branch, so you can also drop that. :D

ack.

@hellwolf
Copy link
Contributor Author

The url to the hevm patch is unstable. Can you please change that to an explicit commit range?

Yes, that makes sense. Done.

Also can you please add a comment to the upstream PR?

It doesn't have an upstream PR for this minimum patch, though. I intend to create a PR with a bigger changeset. Is that okay?

@maralorn
Copy link
Member

The url to the hevm patch is unstable. Can you please change that to an explicit commit range?

Yes, that makes sense. Done.

Also can you please add a comment to the upstream PR?

It doesn't have an upstream PR for this minimum patch, though. I intend to create a PR with a bigger changeset. Is that okay?

Sure.

maralorn
maralorn reviewed Jun 23, 2024
View reviewed changes
pkgs/development/haskell-modules/configuration-common.nix Outdated
hash = "sha256-cL26HD77vXsiKqo5G6PXgK0q19MUGMwaNium5x93CBI=";
}))
(overrideCabal (old: {
postPatch = old.pstPatch or "" + ''
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
postPatch = old.pstPatch or "" + ''
postPatch = old.postPatch or "" + ''

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@hellwolf hellwolf changed the title haskell-updates fixes: hevm, hourglass and crypton-x509 haskell-updates fixes: hevm Jun 23, 2024
@maralorn maralorn merged commit aa96669 into NixOS:haskell-updates Jun 23, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maralorn maralorn maralorn left review comments

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

@ncfavier ncfavier Awaiting requested review from ncfavier

Assignees
No one assigned
Labels
6.topic: haskell 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.
2 participants
@hellwolf @maralorn