sdrpp: fix darwin build & server mode, add options, simplify#452963
Conversation
e7fd439 to
ab63884
Compare
|
There was a problem hiding this comment.
Taking a look at hydra, cubicsdr and pothos are failing already, and the other packages failing are dependants. So it's not this PRs fault.
sdrpp built fine and also the --server mode is working properly! I was planning to open a PR to fix it (following what the current solution does) but was waiting for the other open PRs to be closed. Anyway, your solution looks much more clean, thanks a lot.
However, shouldn't the commit and PR names be sdrpp: 1.1.0-unstable-2024-01-22 -> 1.2.1-unstable-2025-10-06, as per pkgs/README.md#commit-conventions and pkgs/README.md#Versioning?
Other comments were made on the changes.
682b790 to
e7cb262
Compare
|
e7cb262 to
f7f2c60
Compare
|
@rafaelrc7, while I was at it, I decided to change the patch a bit: https://github.com/AlexandreRouma/SDRPlusPlus/compare/master...zaninime:SDRPlusPlus:adapt-nix?expand=1 The differences are:
Do you think it's worth adding to this PR? |
f7f2c60 to
b7b3602
Compare
The code seems fine, and I guess does help experimenting with other modules, so I think it is worth it. |
rafaelrc7
left a comment
There was a problem hiding this comment.
I forgot to submit the review
d457504 to
4119720
Compare
|
Okay, I guess it should be really ready :) |
|
I believe there is something wrong with the patch. I tried running it through However, when I tried to run the binary it errored: You can reproduce it by running |
|
I can't reproduce it. Could it be that your |
You are right. I deleted my config.json file and tried again and it worked. It seems that the patch is fine, but maybe it is incompatible with previous behaviour? If that's true, this surpasses my nixpkgs knowledge and I don't know if this makes this update supposed to be marked as "breaking". Anyway, with that sorted, I think the code itself is fine now. |
|
|
Also, for further referece, I forgot to try the However, when I ran it again it worked, and it did not crash again. I don't know the reason, there might be an edge-case bug in your patch, but, as I mentioned, after the first time crash, it did not happen again. |
|
Mmh, I'm also not able to reproduce it. What I observed is that the software crashes when it's not finding any module, which is consistent with the warning message you got ( But apparently the software fixed the configuration afterwards all by itself? Also the path mentioned is weird. |
4119720 to
504bb00
Compare
|
Rebased atop master due to conflicts from #450737 |
|
504bb00 to
072ee60
Compare
|
@sikmir any outstanding concerns? Could I ask you to otherwise please merge? |
| soapyremote | ||
| soapyrtlsdr | ||
| ] | ||
| ++ (lib.optionals stdenv.hostPlatform.isLinux [ | ||
| soapybladerf |
There was a problem hiding this comment.
We can fix soapybladerf build on darwin by fixing libbladeRF:
--- i/pkgs/by-name/li/libbladeRF/package.nix
+++ w/pkgs/by-name/li/libbladeRF/package.nix
@@ -62,7 +62,7 @@ stdenv.mkDerivation rec {
];
env = lib.optionalAttrs stdenv.cc.isClang {
- NIX_CFLAGS_COMPILE = "-Wno-error=unused-but-set-variable";
+ NIX_CFLAGS_COMPILE = "-Wno-error=unused-but-set-variable -Wno-error=tautological-overlap-compare";
};
hardeningDisable = [ "fortify" ];There was a problem hiding this comment.
Should this be part of this PR? Happy to open a separate one for that.
There was a problem hiding this comment.
Added as a separate commit to this PR.
Allow clean-build of soapysdr-with-plugins on Darwin.
- rework patch to apply more cleanly and cover all entrypoints in one place. - add `audio_source` parameter, useful for making "server only" builds (avoid dependency on rtaudio in these builds) - fix build on Darwin by disabling soapyplutosdr, which in turns depends on libiio (Linux only)
072ee60 to
fc62fcb
Compare
|
The PR you linked is not ready. They are still waiting on uhd to catch up. Anything else here? |
|
|
||
| env = lib.optionalAttrs stdenv.cc.isClang { | ||
| NIX_CFLAGS_COMPILE = "-Wno-error=unused-but-set-variable"; | ||
| NIX_CFLAGS_COMPILE = "-Wno-error=unused-but-set-variable -Wno-error=tautological-overlap-compare"; |
There was a problem hiding this comment.
Maybe we should just omit Werror that is somewhere in the source code.
There was a problem hiding this comment.
Na, this is just a warning clang per default treats as error.
| version_number = "1.2.1"; | ||
|
|
||
| version = "${version_number}-unstable-" + git_date; | ||
| upstreamVersion = "1.2.1"; |
There was a problem hiding this comment.
I would call this releaseVersion but details.
2 commits:
The second required reworking the previous patch and altering the packaging slightly, also to include new options.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.
/cc @sikmir