Skip to content

Comments

electrum: add patch for aiorpcx 0.24 compatibility#380708

Merged
prusnak merged 3 commits intoNixOS:masterfrom
Yarny0:electrum-fixup-aiorpcx
Feb 10, 2025
Merged

electrum: add patch for aiorpcx 0.24 compatibility#380708
prusnak merged 3 commits intoNixOS:masterfrom
Yarny0:electrum-fixup-aiorpcx

Conversation

@Yarny0
Copy link
Contributor

@Yarny0 Yarny0 commented Feb 9, 2025

electrum build broke due to the aiorpcx update in 4bbf530 via #375144 .

Upstream already has a patch, however, it's not so easy to apply:

  • the patch patches the file /run_electrum in the source repo
  • in the source repo, there is a symlink /electrum/electrum pointing to ../run_electrum, i.e., to the file that is patched
  • in the source distribution tarball used by nixpkgs, /electrum/electrum is a simple copy of /run_electrum (probably the result of whatever process generates the tarball)
  • the file /electrum/electrum is used as executable in the package output
  • besides, the patch also attempts to patch the file requirements.txt, but fails with conflicts

Hence, although the patch will likely fix the next electrum release, just applying it to our source does not fix the issue. To work around that, this pull request also adds a copy command to the postFixup section to copy the patched /run_electrum over the unpatched /electrum/electrum. This is also explained in a comment near the fetchpatch line, so whoever removes the patch when it is no longer needed is also reminded of removing the copy command below. applies the changes from the patch file "by hand", with substituteInPlace. Additionally, it converts existing substituteInPlace invocations to --replace-fail and drop two unneeded replacements (see the commits for details).

Note: #309576 also adds a (also temporary) patch for aiorpcx compatibility and experienced the same problem described above, and employed the same solution.

Notifying maintainers: @joachifm @np @prusnak @chewblacka

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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
  • Fits CONTRIBUTING.md.

More: Tested few electrum functionality

  • electrum starts
  • electrum lists known transaction
  • electrum lists known addresses
  • "Electrum" shows up in the Xfce start menu, which starts electrum as expected

nixpkgs-review tries to rebuild 2820 packages and was therefore skipped.


Add a 👍 reaction to pull requests you find important.

@prusnak
Copy link
Member

prusnak commented Feb 9, 2025

posPatches patches contrib/requirements/requirements.txt which you claim does not exist

is that true?

@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 9, 2025
@Yarny0 Yarny0 force-pushed the electrum-fixup-aiorpcx branch from fe9f72b to db902db Compare February 9, 2025 20:13
@Yarny0
Copy link
Contributor Author

Yarny0 commented Feb 9, 2025

posPatches patches contrib/requirements/requirements.txt which you claim does not exist

is that true?

Oh damn, you're absolutely right, @prusnak I'm not sure how I grew the impression that this file does not exist in the tarball. It does! But the patch cannot be applied to that file -- likely due to spesmilo/electrum@1ee6361 which the patch is based upon and which changes a line nearby.

I tried to apply that conflicting commit as well, but it caused other conflicts.

I guess that the requirements.txt file in contrib directory does not affect the functionality of electrum, so I suggest to still simply exclude that file from the patch. Alternatively we could include our own patch.

I've nevertheless updated the branch so the comment for the excludes directive is correct.

What do you think?

@prusnak
Copy link
Member

prusnak commented Feb 9, 2025

I guess that the requirements.txt file in contrib directory does not affect the functionality of electrum, so I suggest to still simply exclude that file from the patch. Alternatively we could include our own patch.

It is opened from setup.py to install dependencies.

I suggest you just drop the fetchpatch and use substituteInPlace to update ./contrib/requirements/requirements.txt and electrum/electrum. While you are at it, I suggest to replace --replace with --replace in every call of substituteInPlace

@Yarny0 Yarny0 force-pushed the electrum-fixup-aiorpcx branch from db902db to d2618f6 Compare February 10, 2025 15:47
@Yarny0
Copy link
Contributor Author

Yarny0 commented Feb 10, 2025

I suggest you just drop the fetchpatch and use substituteInPlace to update ./contrib/requirements/requirements.txt and electrum/electrum. While you are at it, I suggest to replace --replace with --replace in every call of substituteInPlace

I guess you ment --replace-fail and adapted the branch. It now also removes two useless --replace by reverting an old commit, and uses substituteInPlace instead of the patch file. The changed electrum.desktop still works (shows up in Xfce start menu and starts electrum).

Copy link
Member

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

all good except one more fix in the desktop file

This reverts commit a879c72.

It is no longer needed,
as electrum removed the PATH manipulation:
spesmilo/electrum@f15abd7

However, this commit also deviates from a plain revert,
as it adds an additional replacement command to still
substitute the `--testnet` command line, as it was done before.

Note:
Those `--replace` calls are removed here in order to stricten
substitute calls with `--replace-fail` in a follow-up commit.
The old `--replace` is deprecated,
we use `--replace-fail` so changes in source code that
might invalidate substitute invocations get noticed.

Cf. commit e07a2fa .
electrum is quite strict about the aiorpcx version is accepts.
With commit 4bbf530 ,

Upstream already has a patch to extend
the range of aiorpcx versions it permits:
spesmilo/electrum@171aa5e
As the patch does not cleanly apply to the
distribution tarball we're using, we instead apply the
required changes "by hand", with `substituteInPlace`.
On top of the changes adapted from the upstream commit,
we have to change the file `./electrum/electrum`:
It is not part of the upstream source repository,
but it gets created from `./run_electrum` when
upstream creates the distribution tarball.

Note that the commit at hand is similar to
0e513f0 .
@Yarny0 Yarny0 force-pushed the electrum-fixup-aiorpcx branch from d2618f6 to e3d7bba Compare February 10, 2025 16:19
@prusnak prusnak merged commit dffcaa4 into NixOS:master Feb 10, 2025
26 of 29 checks passed
@prusnak
Copy link
Member

prusnak commented Feb 10, 2025

thank you!

@Yarny0 Yarny0 deleted the electrum-fixup-aiorpcx branch February 10, 2025 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants