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

bitcoinarmory: build correctly from source, using *our* libs #29977

Closed
wants to merge 1 commit into from

Conversation

michalrus
Copy link
Member

Motivation for this change

Fixes #29956

To build it correctly, the messy upstream repo requires a patch of:

 22 files changed, 63 insertions(+), 68 deletions(-)

But a relatively simple one.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

/cc @elitak

One issue I’m seeing is this message sometimes on Armory’s stdout/err:

-ERROR - : (StringSockets.cpp:351) FcgiSocket::writeAndRead FcgiError: unexpected fcgi header version

… but it happens using both the newest libfcgi, and the old version that BitcoinArmory wants. Here’s the patch that uses the old version, in case anyone needs it:

diff --git a/pkgs/applications/misc/bitcoinarmory/default.nix b/pkgs/applications/misc/bitcoinarmory/default.nix
index 22974a3f57..82dd753272 100644
--- a/pkgs/applications/misc/bitcoinarmory/default.nix
+++ b/pkgs/applications/misc/bitcoinarmory/default.nix
@@ -6,6 +6,27 @@ let
 
   inherit (pythonPackages) mkPythonDerivation pyqt4 psutil twisted;
 
+  #
+  # BitcoinArmory uses an ancient version of libfcgi. With our current
+  # Nixpkgs’ one, the following assertion fails in runtime:
+  #
+  # -ERROR - : (StringSockets.cpp:351) FcgiSocket::writeAndRead FcgiError: unexpected fcgi header version
+  #
+  # Let’s build that ancient version, then:
+  #
+  fcgi1 = stdenv.mkDerivation rec {
+    name = "fcgi-${version}";
+    version = "1.21";
+    nativeBuildInputs = [ pkgconfig autoreconfHook ];
+    src = fetchFromGitHub {
+      owner = "goatpig";
+      repo = "libfcgi";
+      rev = "b00dc69199b34552144e1003a2001a24b13b6e56";
+      sha256 = "1d97k3hjrf3798ks358ycn1v8frykc6f96lf47253m1hcdbp2m3k";
+    };
+    postInstall = "ln -s . $out/include/fastcgi";
+  };
+
 in mkPythonDerivation rec {
 
   name = "bitcoinarmory-${version}";
@@ -48,7 +69,7 @@ in mkPythonDerivation rec {
 
   nativeBuildInputs = [ pkgconfig autoreconfHook ];
 
-  buildInputs = [ swig qt4 fcgi cryptopp lmdb ];
+  buildInputs = [ swig qt4 fcgi1 cryptopp lmdb ];
 
   propagatedBuildInputs = [ pyqt4 psutil twisted ];
 

@elitak
Copy link
Contributor

elitak commented Oct 1, 2017

I confirmed this fixes the build issues. Looks good, except that git log -p shows a lot of stray/trailing whitespace that could be cleaned and the version bump should be separated into its own commit.

Thanks a lot for this. Fixing obnoxious codebases like this one is a real chore, so I appreciate your effort.

@jb55
Copy link
Contributor

jb55 commented Oct 1, 2017

That's quite the patch to maintain, any chance of convincing upstream to make some changes?

@michalrus
Copy link
Member Author

michalrus commented Oct 1, 2017

@elitak, I left it out purposefully, not to bloat the patch even more, to keep it minimal. Thank you for the kind words! 😊

@jb55, I… d-don’t think so, given the specifics of that community, but if you want to try… 😞 *But*! Notice, that this patch is mostly changing include "something.h" to a correct include <cryptopp/something.h> — I wouldn’t expect those fragments to change very often. If you look into git-blame of upstream, they indeed don’t. 😊

@michalrus
Copy link
Member Author

michalrus commented Oct 1, 2017

Also, I think we might need to apply this fcgi-1.21 patch above. :/ @elitak, does Armory work correctly for you w/ the newest fcgi? Hmm.

@elitak
Copy link
Contributor

elitak commented Oct 1, 2017

The basics work. I haven't seen that fcgi version mismatch error on the console. I'm not sure what triggers it. The detection of the satoshis/byte fee measurement still doesn't work, but I have no idea if that's to do with libfcgi or not.

@michalrus
Copy link
Member Author

Related: goatpig/BitcoinArmory#324

@jb55
Copy link
Contributor

jb55 commented Oct 3, 2017

maybe just throw that link next to the patches line as a note if it ever gets fixed

@michalrus
Copy link
Member Author

goatpig/BitcoinArmory#324 (comment) 🤦‍♂️

I think I give up. @elitak, @jb55 feel free to take this over. *smh*

@jb55
Copy link
Contributor

jb55 commented Oct 3, 2017

Thanks for your effort. If he has modified them locally maybe this patch might not be the best way to go about it. but that still raises the question as to why was it crashing originally...

@michalrus
Copy link
Member Author

Uh-huh!

And also: if he modified general-purpose crypo libraries in place, then maybe this application should not be used for anything serious. 😃

@elitak
Copy link
Contributor

elitak commented Oct 10, 2017

More bad news: my build using this patch has issues syncing with bitcoind and checking new blocks, making it pretty much unusable for anything outside of offline transactions, probably. I think it's best we shelve this for now. I'll revisit this if I see a lot of upstream activity. Thanks again.

@Ekleog
Copy link
Member

Ekleog commented Oct 5, 2018

(triage) From what I read, no one is actually working on this PR… maybe close and open an issue instead, as it appears the direction of the original patch was problematic?

@jb55
Copy link
Contributor

jb55 commented Oct 5, 2018

yeah this can be closed

@veprbl
Copy link
Member

veprbl commented Jan 12, 2019

Unfortunately, this won't fly because: goatpig/BitcoinArmory#324 (comment)

@veprbl veprbl closed this Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants