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

Add teos package (The Eye of Satoshi) with clightning watchtower plugin #195782

Merged
merged 2 commits into from
Oct 14, 2022

Conversation

seberm
Copy link
Contributor

@seberm seberm commented Oct 13, 2022

Description of changes

I would like to add teos package (implementation of lightning watchtower) into nixpkgs:

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.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Oct 13, 2022
@seberm seberm changed the title Add teos package (The Eye of Satoshi) witch clightning watchtower plugin Add teos package (The Eye of Satoshi) with clightning watchtower plugin Oct 13, 2022
@seberm seberm marked this pull request as ready for review October 13, 2022 10:01
@prusnak
Copy link
Member

prusnak commented Oct 13, 2022

Don't we want to add teos-watchtower-plugin to all-packages.nix?

@seberm
Copy link
Contributor Author

seberm commented Oct 13, 2022

Hello Pavol,

Don't we want to add teos-watchtower-plugin to all-packages.nix?

Sure we want. I've made the changes. Thanks!

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Oct 13, 2022
@prusnak
Copy link
Member

prusnak commented Oct 13, 2022

Build error on darwin:

ld: framework not found Security

Please add Security conditionally for darwin to buildInputs and add the following to all-packages.nix:

inherit (darwin.apple_sdk.frameworks) Security;

@prusnak
Copy link
Member

prusnak commented Oct 13, 2022

@seberm
Copy link
Contributor Author

seberm commented Oct 13, 2022

Hm, Darwin still fails :-/ https://logs.nix.ci/?key=nixos/nixpkgs.195782&attempt_id=6274d55b-bc05-4147-bd1b-872de81c7dc7

Hm, strange :/. Is it somehow possible to reproduce this issue on my (x86_64) machine so I can debug it more easily?

@prusnak
Copy link
Member

prusnak commented Oct 14, 2022

Tried this on aarch64-darwin and got lucky. My compilation output contained more info:

  cargo:warning=curl/lib/hostip.c:72:10: fatal error: 'SystemConfiguration/SCDynamicStoreCopySpecific.h' file not found
  cargo:warning=#include <SystemConfiguration/SCDynamicStoreCopySpecific.h>
  cargo:warning=         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  cargo:warning=1 error generated.
  exit status: 1

leading to the following fix:

diff --git a/pkgs/applications/blockchains/teos/default.nix b/pkgs/applications/blockchains/teos/default.nix
index c92b2ebfa34..0fbb968a66e 100644
--- a/pkgs/applications/blockchains/teos/default.nix
+++ b/pkgs/applications/blockchains/teos/default.nix
@@ -8,6 +8,7 @@
 , protobuf
 , rustfmt
 , Security
+, SystemConfiguration
 }:
 
 let
@@ -31,7 +32,7 @@ let
 
   buildInputs = [
     openssl
-  ] ++ lib.optionals stdenv.isDarwin [ Security ];
+  ] ++ lib.optionals stdenv.isDarwin [ Security SystemConfiguration ];
 
   nativeBuildInputs = [
     perl  # used by openssl-sys to configure
diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index ffbf0271ac6..e0edf27318e 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -33234,7 +33234,7 @@ with pkgs;
   taro = callPackage ../applications/blockchains/taro { };
 
   inherit (callPackages ../applications/blockchains/teos {
-    inherit (darwin.apple_sdk.frameworks) Security;
+    inherit (darwin.apple_sdk.frameworks) Security SystemConfiguration;
   })
     teos
     teos-watchtower-plugin;

So the build now works fine, however, the tests fail with:

...
test retrier::tests::test_manage_retry_unreachable ... FAILED
...
---- retrier::tests::test_manage_retry_unreachable stdout ----
thread 'retrier::tests::test_manage_retry_unreachable' panicked at 'assertion failed:
wt_client.lock().unwrap().towers.get(&tower_id).unwrap().status.is_unreachable()', watchtower-plugin/src/retrier.rs:518:9

Is there a way how to skip a single test in buildRustPackage?

If not, let's just skip the tests for aarch64-darwin with doCheck = !(stdenv.isDarwin && stdenv.isAarch64); (tests on x86_64-darwin pass)

In both cases of skipping the test, let's add the error output above as a comment to indicate why we skipped the tests.

@prusnak
Copy link
Member

prusnak commented Oct 14, 2022

@GrahamcOfBorg build teos teos-watchtower-plugin

@prusnak
Copy link
Member

prusnak commented Oct 14, 2022

Tested 2f4f8e6 on {aarch64,x86_64}-darwin locally - it builds and works. Let's wait for the CI, then I'll merge. 👍

@seberm
Copy link
Contributor Author

seberm commented Oct 14, 2022

Thank you for helping me with the testing!

I have contacted teos developers via their discord channel and I asked them to take a look at the failing test.

CC: @sr-gi

@sr-gi
Copy link

sr-gi commented Oct 14, 2022

Hey guys

...
test retrier::tests::test_manage_retry_unreachable ... FAILED
...
---- retrier::tests::test_manage_retry_unreachable stdout ----
thread 'retrier::tests::test_manage_retry_unreachable' panicked at 'assertion failed:
wt_client.lock().unwrap().towers.get(&tower_id).unwrap().status.is_unreachable()', watchtower-plugin/src/retrier.rs:518:9

That check is performed after a sleep to externally check the state of the retrier. It could be the case that the sleep is not big enough depending on the architecture and the check is performed before the state of the retrier has changed :S

Do you have any good way of reproducing this? I'd love to test this myself, and even add darwin+aarch64 to our test matrix

@prusnak
Copy link
Member

prusnak commented Oct 14, 2022

Do you have any good way of reproducing this?

I am not able to reproduce on aarch64-darwin with the official cargo/rust binaries. Only with the nixpkgs ones. Let's merge this PR as is and we can investigate the issue later.

@prusnak prusnak merged commit 401e3d5 into NixOS:master Oct 14, 2022
@prusnak
Copy link
Member

prusnak commented Oct 14, 2022

Hm, I must have missed something earlier. Now I can reproduce by simply running the following on an Apple Silicon hardware (Macbook Air M1):

cd rust-teos
cargo build
cargo test
$ cargo --version
cargo 1.64.0 (387270bc7 2022-09-16)
$ rustc --version
rustc 1.64.0 (a55dd71d5 2022-09-19)

@sr-gi
Copy link

sr-gi commented Oct 17, 2022

Hm, I must have missed something earlier. Now I can reproduce by simply running the following on an Apple Silicon hardware (Macbook Air M1):

cd rust-teos
cargo build
cargo test
$ cargo --version
cargo 1.64.0 (387270bc7 2022-09-16)
$ rustc --version
rustc 1.64.0 (a55dd71d5 2022-09-19)

Any clue on how to run this without physically having an Apple Silicon mac? Looks like Githbub Actions do not support it yet and I don't have one myself to test it out :S

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants