gnss-sdr: init at 0.0.9 (and patch armadillo's buildInputs) #27505

jameysharp · 2017-07-19T19:29:55Z

Motivation for this change

This open-source implementation of a software-defined radio receiver for GPS and other GNSS signals was not yet packaged.

In order to get a usable build of gnss-sdr, I had to change armadillo's buildInputs to add liblapack; otherwise several important functions fail at runtime. Perhaps @juliendehos or @knedlsepp will want to comment on that change?

I've tested the resulting gnss-sdr binary against the sample 2013_04_04_GNSS_SIGNAL_at_CTTC_SPAIN dataset linked from http://gnss-sdr.org/my-first-fix/.

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.

knedlsepp · 2017-07-19T20:46:13Z

Afaik openblas should include all lapack routines, so I think we should check why armadillo depends on an extra library for that.

jameysharp · 2017-07-19T21:44:35Z

Afaik openblas should include all lapack routines, so I think we should check why armadillo depends on an extra library for that.

Huh. All I know so far is that the armadillo build reports "LAPACK_FOUND = NO" (for instance in http://hydra.nixos.org/build/56592131/log), and that adding liblapack to its buildInputs is all that's necessary to make it not die with error messages like "svd(): use of LAPACK must be enabled", which are reported using arma_stop_logic_error.

Any suggestions on how to investigate this?

(Also, I botched my fetchurl call, so I've pushed a revised version that should actually build this time...)

juliendehos · 2017-07-20T07:06:19Z

Openblas includes an optimized implementation of Lapack. Opencv (with Openblas enabled) finds it:

...
--     Use Lapack:                  YES (/nix/store/priv29ycsiagyc4f9pamblym16v4az16-openblas-0.2.19/lib/libopenblas.so)
...

knedlsepp · 2017-07-20T20:19:58Z

It seems armadillo doesn't know about openBLAS's lapack functionality yet. I've filed an issue:
https://github.com/conradsnicta/armadillo-code/issues/30
In the meantime we could fix the armadillo build via:

  cmakeFlags = let
    libSuff = if stdenv.isDarwin then "dylib" else "so"; 
  in [ 
    "-DLAPACK_LIBRARY=${openblasCompat}/lib/libopenblas.${libSuff}"
    "-DDETECT_HDF5=ON" 
  ];

I've created a PR for this: #27528

knedlsepp · 2017-07-20T21:33:38Z

Looking at the commit history for our armadillo derivation I just realized that this should have been fixed long ago, but I accidentally reverted it: https://github.com/NixOS/nixpkgs/commits/82e760a3d613ec9e82ab48131f8b94b1119dc535/pkgs/development/libraries/armadillo/default.nix

conradsnicta · 2017-07-21T01:20:39Z

@jameysharp @juliendehos @knedlsepp

Unfortunately it can't be guaranteed that OpenBLAS comes with LAPACK. Last time I checked, the super-wise folks at Debian build their version of OpenBLAS without LAPACK. Debian stuff then propagates to Ubuntu, and we end up with a royal mess.

Armadillo needs to support various Linux-based operating systems, so it can't assume that OpenBLAS comes with LAPACK built in.

Armadillo's cmake installer simply enables LAPACK support if LAPACK is detected. If an untainted version of OpenBLAS is on the system, it should use LAPACK functions from OpenBLAS instead of standard LAPACK. This is due to the linker simply finding the appropriate functions in OpenBLAS first.

In other words, in addition to OpenBLAS, I suggest having a standard version of LAPACK on the system.

juliendehos · 2017-07-21T08:32:08Z

just using openblasCompat instead of openblas seems to "solve" the problem :

//testarma.cpp
#define ARMA_USE_LAPACK
#define ARMA_DONT_USE_WRAPPER
#include <armadillo>
using namespace arma;
int main() {
    mat U, V, X = randu<mat>(9, 9);
    vec s;
    svd(U,s,V,X);
    return 0;
}

#default.nix
with import <nixpkgs> {};
with pkgs; stdenv.mkDerivation {
    name = "testarma";
    src = ./.;
    buildInputs = [ armadillo openblasCompat ];  # ok 
    #buildInputs = [ armadillo openblas ];       # svd(): decomposition failed
    buildPhase = "g++ -std=c++11 -o testarma testarma.cpp -larmadillo -lopenblas";
    installPhase = "mkdir -p $out/bin; cp testarma $out/bin";
 }

knedlsepp · 2017-07-21T09:07:02Z

@juliendehos: The fix I'm proposing here in nixpkgs would mean that you could get rid of:

#define ARMA_USE_LAPACK
#define ARMA_DONT_USE_WRAPPER

and -lopenblas. If we add this fix the ARMA_USE_LAPACK would globally be encoded in the armadillo installation and linking with -larmadillo would transitively link the openBLAS routines.

juliendehos · 2017-07-21T09:20:42Z

Ok, good idea. I've tested your fix and it seems ok. Thanks.

knedlsepp · 2017-07-22T06:19:17Z

@jameysharp: Could you rebase your PR to exclude the armadillo lapack addition, now that the fix was merged? (#27528)

jameysharp · 2017-07-22T07:04:47Z

I've rebased and re-tested just adding gnss-sdr, dropping my patch to armadillo, and with the version of armadillo that's on master everything works great. Thank you!

I also finally turned on nixos.useSandbox, so AFAICT this test build was even sandboxed.

Got any other concerns before merging this?

knedlsepp · 2017-07-22T07:17:55Z

pkgs/applications/misc/gnss-sdr/default.nix

@@ -0,0 +1,61 @@
+{ stdenv, fetchFromGitHub
+, armadillo
+, blas


I'm a little skeptical if including blas and lapack as buildInputs is correct. armadillo links to a different BLAS version (our openblasCompat). I suspect that they should be purged.

A fair concern, but without them the build fails with one of these two messages:

The LAPACK library has not been found. You can try to install it by typing: sudo apt-get install liblapack-dev CMake Error at CMakeLists.txt:762 (message): LAPACK is required to build gnss-sdr

or

The BLAS library has not been found. You can try to install it by typing: sudo apt-get install libopenblas-dev CMake Error at CMakeLists.txt:783 (message): BLAS is required to build gnss-sdr

Also, I tested building with both openblasCompat and openblas, and the configure script failed to detect either one. It only accepts blas. Which is funny since the Debian package it helpfully suggests installing is for openblas...

I don't understand any of the relevant code well enough to guess what the right fix is. In particular, I haven't worked with lapack/blas before, but I'm also not super comfortable with cmake. Any suggestions?

Looking at their CMakelists.txt. It seems they do the following:

Require CMake finds LAPACK

Require CMake finds BLAS

Try to find armadillo

If it is not found, build it with the previously found LAPACK and BLAS

If it is found, just use it and ignore which LAPACK and BLAS was actually found previously.

So I think that by adding lapack and blas to the derivation, we actually just pass the first two stages and then drop the found lapack and blas. I think we could get this working by just forcing CMake to pass these first two stages via:

cmakeFlags = [ "-DBLAS=YES" "-DLAPACK=YES" ];

Closer look also reveals that the same goes probably for gfortran and -D GFORTRAN=YES

knedlsepp · 2017-07-22T07:57:53Z

pkgs/applications/misc/gnss-sdr/default.nix

+    description = "An open source Global Navigation Satellite Systems software-defined receiver";
+    homepage = http://gnss-sdr.org/;
+    license = licenses.gpl3Plus;
+    platforms = platforms.unix;


I'm trying to build this on darwin, but at least one of the dependencies requires linux currently, so unless we can get it to work on macOS this should be changed to platforms.linux in the meantime.

@knedlsepp

I've tested the resulting build against the sample "2013_04_04_GNSS_SIGNAL_at_CTTC_SPAIN" dataset linked from: http://gnss-sdr.org/my-first-fix/ Many thanks to @knedlsepp for suggestions in NixOS#27505 that have substantially improved this package!

jameysharp · 2017-07-22T20:17:24Z

You're absolutely right @knedlsepp, thanks! I've tested and pushed your suggested fixes. Do you think this is ready to merge now?

knedlsepp · 2017-07-22T23:26:19Z

Looks good to me. 🍻

Mic92 · 2017-07-23T14:12:39Z

Thanks!

jameysharp · 2017-07-23T15:37:53Z

And thank you too, @Mic92!

@knedlsepp

I've tested the resulting build against the sample "2013_04_04_GNSS_SIGNAL_at_CTTC_SPAIN" dataset linked from: http://gnss-sdr.org/my-first-fix/ Many thanks to @knedlsepp for suggestions in NixOS#27505 that have substantially improved this package!

jameysharp force-pushed the gnss-sdr branch from 6fb48c2 to 3298117 Compare July 19, 2017 21:43

jameysharp force-pushed the gnss-sdr branch from 3298117 to 57b8974 Compare July 22, 2017 06:52

knedlsepp reviewed Jul 22, 2017

View reviewed changes

gnss-sdr: init at 0.0.9

22292c0

I've tested the resulting build against the sample "2013_04_04_GNSS_SIGNAL_at_CTTC_SPAIN" dataset linked from: http://gnss-sdr.org/my-first-fix/ Many thanks to @knedlsepp for suggestions in NixOS#27505 that have substantially improved this package!

jameysharp force-pushed the gnss-sdr branch from 57b8974 to 22292c0 Compare July 22, 2017 20:12

knedlsepp approved these changes Jul 22, 2017

View reviewed changes

Mic92 merged commit 11f8524 into NixOS:master Jul 23, 2017

jameysharp deleted the gnss-sdr branch July 8, 2019 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gnss-sdr: init at 0.0.9 (and patch armadillo's buildInputs) #27505

gnss-sdr: init at 0.0.9 (and patch armadillo's buildInputs) #27505

jameysharp commented Jul 19, 2017

knedlsepp commented Jul 19, 2017 via email •

edited

jameysharp commented Jul 19, 2017

juliendehos commented Jul 20, 2017

knedlsepp commented Jul 20, 2017 •

edited

knedlsepp commented Jul 20, 2017

conradsnicta commented Jul 21, 2017

juliendehos commented Jul 21, 2017 •

edited

knedlsepp commented Jul 21, 2017 •

edited

juliendehos commented Jul 21, 2017

knedlsepp commented Jul 22, 2017 •

edited

jameysharp commented Jul 22, 2017

knedlsepp Jul 22, 2017

jameysharp Jul 22, 2017

knedlsepp Jul 22, 2017 •

edited

knedlsepp Jul 22, 2017

knedlsepp Jul 22, 2017

jameysharp commented Jul 22, 2017

knedlsepp commented Jul 22, 2017

Mic92 commented Jul 23, 2017

jameysharp commented Jul 23, 2017

gnss-sdr: init at 0.0.9 (and patch armadillo's buildInputs) #27505

gnss-sdr: init at 0.0.9 (and patch armadillo's buildInputs) #27505

Conversation

jameysharp commented Jul 19, 2017

Motivation for this change

Things done

knedlsepp commented Jul 19, 2017 via email • edited

jameysharp commented Jul 19, 2017

juliendehos commented Jul 20, 2017

knedlsepp commented Jul 20, 2017 • edited

knedlsepp commented Jul 20, 2017

conradsnicta commented Jul 21, 2017

juliendehos commented Jul 21, 2017 • edited

knedlsepp commented Jul 21, 2017 • edited

juliendehos commented Jul 21, 2017

knedlsepp commented Jul 22, 2017 • edited

jameysharp commented Jul 22, 2017

knedlsepp Jul 22, 2017

Choose a reason for hiding this comment

jameysharp Jul 22, 2017

Choose a reason for hiding this comment

knedlsepp Jul 22, 2017 • edited

Choose a reason for hiding this comment

knedlsepp Jul 22, 2017

Choose a reason for hiding this comment

knedlsepp Jul 22, 2017

Choose a reason for hiding this comment

jameysharp commented Jul 22, 2017

knedlsepp commented Jul 22, 2017

Mic92 commented Jul 23, 2017

jameysharp commented Jul 23, 2017

knedlsepp commented Jul 19, 2017 via email •

edited

knedlsepp commented Jul 20, 2017 •

edited

juliendehos commented Jul 21, 2017 •

edited

knedlsepp commented Jul 21, 2017 •

edited

knedlsepp commented Jul 22, 2017 •

edited

knedlsepp Jul 22, 2017 •

edited