satdump: init at 1.2.2 #369540
satdump: init at 1.2.2 #369540
Conversation
github-actions
bot
added
the
8.has: maintainer-list (update)
NixOSInfra
added
the
12. first-time contribution
github-actions
bot
added
10.rebuild-darwin: 0
theverygaming
force-pushed
the
satdump
branch
from
13ff8b7 to
e41edbd
Compare
|
theverygaming
force-pushed
the
satdump
branch
from
e41edbd to
eaa37ee
Compare
github-actions
bot
added
10.rebuild-darwin: 1-10
and removed
10.rebuild-darwin: 0
|
|
@lucasew running |
It is happening sometimes |
|
I'll let other people properly review this, but it compiles and works with an rtl-sdr for me, so LGTM and thank you for the packaging effort! |
|
pkgs/by-name/sa/satdump/package.nix
Outdated
| --replace-fail '$'{CMAKE_INSTALL_PREFIX}/'$'{CMAKE_INSTALL_LIBDIR} '$'{CMAKE_INSTALL_FULL_LIBDIR} | ||
| ''; | ||
|
|
||
| passthru.updateScript = gitUpdater { }; |
There was a problem hiding this comment.
I'd recommend using nix-update-script instead, as it is more robust.
pkgs/by-name/sa/satdump/package.nix
Outdated
| description = "A generic satellite data processing software"; | ||
| homepage = "https://www.satdump.org/"; | ||
| changelog = "https://github.com/SatDump/SatDump/releases/tag/${finalAttrs.version}"; | ||
| license = lib.licenses.gpl3Only; |
There was a problem hiding this comment.
| license = lib.licenses.gpl3Only; | |
| license = lib.licenses.gpl3Plus; |
The license contains the "or (at your option) any later version" clause
| # Optional dependencies | ||
| withZIQRecordingCompression ? true, | ||
| zstd, | ||
| withGUI ? true, |
There was a problem hiding this comment.
Two things:
- I can't imagine that too many people will be using these option flags. They are typically used when a non-free dependency has to be used to enable the feature, or if the option significantly changes how the program works (e.g. monado's tracing support). This is a lot to maintain, and disk space is relatively cheap nowadays.
- If you decide to keep the flags, can I recommend you put them at the bottom of the parameter list so they can be differentiated from dependencies at a glance?
There was a problem hiding this comment.
- I chose to add these options because most of them are SDR sources, realistically people may only own a small subset of the receivers the software supports, so they may prefer to keep some of them disabled to save disk space. I can see SatDump being used in more embedded-ish applications where disk space may actually matter
- Each of the flags is above the dependencies it enables, which i think is more readable than having the dependencies alltogether above the flags
There was a problem hiding this comment.
I can see SatDump being used in more embedded-ish applications where disk space may actually matter
I decided to try to measure the closure size of the dependencies you are concerned about. The full derivation, including all of the dependencies, is 7.53 MiB. I'll agree that withGUI makes sense to keep; disabling it should reduce the closure size by about 3 MiB... if it built correctly:
❯ nix build /nix/store/g23w6778khi5w333lgbfk7fcqy65cjys-satdump-1.2.2.drv^*
error: builder for '/nix/store/g23w6778khi5w333lgbfk7fcqy65cjys-satdump-1.2.2.drv' failed with exit code 1;
last 25 log lines:
> -- Found JEMALLOC headers: /nix/store/5nmvrb919gjjjdqy5xp3rb049ksmgbiz-jemalloc-5.3.0/include
> -- Checking for module 'volk'
> -- Found volk, version 3.1
> -- Performing Test VOLK_HAS_ALLOC
> -- Performing Test VOLK_HAS_ALLOC - Success
> -- Found ZLIB: /nix/store/cqlaa2xf6lslnizyj9xqa8j0ii1yqw0x-zlib-1.3.1/lib/libz.so (found version "1.3.1")
> -- Found PNG: /nix/store/ayk2mh02j7wrvgx17155xcwvpmgrzzka-libpng-apng-1.6.43/lib/libpng.so (found version "1.6.43")
> -- Checking for module 'fftw3f'
> -- Found fftw3f, version 3.3.10
> -- Found CURL: /nix/store/8yfak7dis3yqqls4mclzp5jb1ic2jzab-curl-8.11.0/lib/libcurl.so (found version "8.11.0")
> -- Building with curl!
> -- Performing Test VOLK_HAS_volk_32fc_x2_add_32fc
> -- Performing Test VOLK_HAS_volk_32fc_x2_add_32fc - Success
> Building the GUI
> Building GUI with OpenGL
> CMake Error at /nix/store/38cffsqqx823crf1i4bcf6zz1qz1hgpd-cmake-3.30.5/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
> Could NOT find OpenGL (missing: OPENGL_opengl_LIBRARY OPENGL_glx_LIBRARY
> OPENGL_INCLUDE_DIR)
> Call Stack (most recent call first):
> /nix/store/38cffsqqx823crf1i4bcf6zz1qz1hgpd-cmake-3.30.5/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE)
> /nix/store/38cffsqqx823crf1i4bcf6zz1qz1hgpd-cmake-3.30.5/share/cmake-3.30/Modules/FindOpenGL.cmake:579 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
> src-ui/CMakeLists.txt:28 (find_package)
>
>
> -- Configuring incomplete, errors occurred!
For full logs, run 'nix log /nix/store/g23w6778khi5w333lgbfk7fcqy65cjys-satdump-1.2.2.drv'.
This is my point: adding all these options means a lot more testing is required. This is your first contribution to nixpkgs. Be warned: you do not want to add unnecessary maintenance burden on yourself unless you have a good reason to (i.e. you are actually making the embedded devices in question).
Side note: what embedded devices are you thinking of that run NixOS or linux in general? A raspberry pi compute module or similar would be using an SD card on the order of about a dozen gigabytes, so shaving a few MiB shouldn't matter.
There was a problem hiding this comment.
I have now fixed the build without the GUI, it's the one thing i seem to have forgotten to test.
For the other options, things automatically don't get built when the dependencies are missing - which doesn't seem to be the case for the GUI.
Also the whole thing with dependencies being 7.53 MiB surprises me, that's out of the question then.
I don't think the options are a large extra maintenance effort, and i can also see them being useful in other cases. For exmple if i remember correctly there once was a bug in libhackrf (or maybe it was another SDR library, this was some years ago) that caused connected hackRF devices to be opened (which means they are unusable by other programs) at library initialization. In such cases the option of disabling these sources may prove to be quite useful.
On top of that i do plan to add support for SatDump on darwin where some depdendencies may not be available, i can see the options being useful in that case too.
And a piece of software similar to this one, also with various sources for various SDR devices also has a bunch of options for each source, see
nixpkgs/pkgs/applications/radio/sdrpp/default.nix
Lines 15 to 64 in 5bc9fbb
theverygaming
force-pushed
the
satdump
branch
from
eaa37ee to
1cc0216
Compare
theverygaming
force-pushed
the
satdump
branch
from
1cc0216 to
6cddd2f
Compare
Pandapip1
added
the
12.approvals: 1
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/2195 |
Closes: #314003
Packaged SatDump, a software to receive, decode and process data from satellites.
See https://www.satdump.org/about/ and https://github.com/SatDump/SatDump
Also https://repology.org/project/satdump/versions
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.