flann: refactor #129491

mikepurvis · 2021-07-07T01:56:17Z

Motivation for this change

Flann is an important package in the robotics community, as it is a dependency of PCL, the pointcloud library. Unfortunately, it hasn't received a release since 2016 and the repo has accumulated a large number of open issues and PRs:

https://github.com/flann-lib/flann/tags

Most notably, the 1.9.1 release has an issue where it bundles a copy of the lz4 headers. This change piggy-backs on the already-reviewed patches used in the Debian version of the package, prepared by @jspricke.

Things done

I'm a first-time contributor, so I don't know exactly what is best here, but in my local clone of nixpkgs (on NixOS) I ran:

nix build .#flann

And confirmed a successful build, with enablePython in both possible states.

Am running nixpkgs-review pr 129491, and so far I have the following failure:

nix log /nix/store/0gx2hycrlqdw8lmbh36yqiykmh5m9kj3-hugin-2019.0.0.drv
...
[ 70%] Linking CXX executable cpfind
/nix/store/scs241fp7dlrvm45d2fjbpjvygirn0ml-binutils-2.35.1/bin/ld: CMakeFiles/cpfind.dir/PanoDetectorLogic.cpp.o: undefined reference to symbol 'LZ4_compress_HC_continue'
/nix/store/scs241fp7dlrvm45d2fjbpjvygirn0ml-binutils-2.35.1/bin/ld: /nix/store/3nf85zf44vyvwr32hx53v7swljxysyjm-lz4-1.9.3/lib/liblz4.so.1: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

FYI @lopsided98

r-rmcgibbo · 2021-07-07T03:10:23Z

Result of nixpkgs-review pr 129491 at 4da9729a run on aarch64-linux 1

1 package failed to build:

hugin

2 packages skipped due to time constraints:

digikam
pcl

2 packages built successfully:

cloudcompare
flann

3 suggestions:

warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/development/libraries/flann/default.nix:26:5:
```
   |
26 |     (fetchpatch {
   |     ^
```
warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/development/libraries/flann/default.nix:34:5:
```
   |
34 |     (fetchpatch {
   |     ^
```
warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/development/libraries/flann/default.nix:30:5:
```
   |
30 |     (fetchpatch {
   |     ^
```

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

Result of nixpkgs-review pr 129491 at 4da9729a run on x86_64-linux 1

1 package failed to build:

hugin

2 packages skipped due to time constraints:

digikam
pcl

2 packages built successfully:

cloudcompare
flann

3 suggestions:

warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/development/libraries/flann/default.nix:26:5:
```
   |
26 |     (fetchpatch {
   |     ^
```
warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/development/libraries/flann/default.nix:30:5:
```
   |
30 |     (fetchpatch {
   |     ^
```
warning: missing-patch-comment
Consider adding a comment explaining the purpose of this patch on the line preceeding.
Near pkgs/development/libraries/flann/default.nix:34:5:
```
   |
34 |     (fetchpatch {
   |     ^
```

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

mikepurvis · 2021-07-07T14:57:28Z

So the issue with Hugin is that it uses an included FindFLANN.cmake script that just looks for the sofiles without considering the correctly-generated pkgconfig information:

# This file was generated by CMake for flann
prefix=/nix/store/xrnvfm5f6f9k5lkca0ik3s9z52d84iw3-flann-1.9.1
exec_prefix=${prefix}
libdir=${prefix}/lib
includedir=${prefix}/include

Name: flann
Description: Fast Library for Approximate Nearest Neighbors
Version: 1.9.1
Requires:
Libs: -L${libdir} -L/nix/store/3nf85zf44vyvwr32hx53v7swljxysyjm-lz4-1.9.3/lib;-llz4 -lflann -lflann_cpp
Cflags: -I${includedir}

There is a newer release of Hugin (2020.0), but it doesn't seem to address this issue. I don't see a patch for this in Debian, so I'm not sure how it's working there either. If it needs to be patched, my inclination would be just to grab the FindFLANN.cmake from PCL, since that one does the right thing (including being future-proof to a glorious day when FLANN ships its own find module):

https://github.com/PointCloudLibrary/pcl/blob/master/cmake/Modules/FindFLANN.cmake

jspricke · 2021-07-07T15:15:45Z

@mikepurvis the hugin in Debian is version 2020.0.0 and uses pkg-config and gets -llz4 from there:

-- checking for module 'flann'
--   found flann, version 1.9.1
-- flann library found
...
../../celeste/libceleste.so.0.0 -llz4 -lflann -lflann_cpp ../../hugin_base/libhuginbase.so.0.0 /usr/lib/x86_64-linux-gnu/libX11.so

https://buildd.debian.org/status/fetch.php?pkg=hugin&arch=amd64&ver=2020.0.0%2Bdfsg-2&stamp=1609245416&raw=0

mikepurvis · 2021-07-07T16:06:50Z

@jspricke Thanks for jumping in here! On closer inspection, I see that the FindFLANN module does in fact use pkgconfig:

if(UNIX)
  find_package(PkgConfig QUIET)
  pkg_check_modules(FLANN flann)

And it's unchanged between the 2019 and 2020 versions. Both fail in my local nix develop shell in the same way— it definitely doesn't have the proper link flag for lz4:

cd /home/ciadmin/hugin-src/hugin-2019.0.0/build/src/hugin_cpfind/cpfind && /nix/store/186fa5snplvdlvimx0jgw88ls2v62zxv-cmake-3.19.7/bin/cmake -E cmake_link_script CMakeFiles/cpfind.dir/link.txt --verbose=1
/nix/store/fdy0bnh3x01rysv3cig42wq3sfcqf0zz-gcc-wrapper-10.3.0/bin/g++ -O2 -g -DNDEBUG  -fopenmp -rdynamic CMakeFiles/cpfind.dir/PanoDetector.cpp.o CMakeFiles/cpfind.dir/PanoDetectorLogic.cpp.o CMakeFiles/cpfind.dir/TestCode.cpp.o CMakeFiles/cpfind.dir/Utils.cpp.o CMakeFiles/cpfind.dir/main.cpp.o -o cpfind  -Wl,-rpath,/home/ciadmin/hugin-src/hugin-2019.0.0/build/src/hugin_cpfind/localfeatures:/home/ciadmin/hugin-src/hugin-2019.0.0/build/src/celeste:/home/ciadmin/hugin-src/hugin-2019.0.0/build/src/hugin_base: ../localfeatures/liblocalfeatures.so.0.0 -lvigraimpex -lImath -lIlmImf -lIex -lHalf -lIlmThread -lz -ljpeg -ltiff -lpng -lz -lz -lexiv2 -lpano13 ../../foreign/levmar/libhuginlevmar.a -lGLEW -lboost_filesystem -lboost_system -lfftw3 -lvigraimpex -lImath -lIlmImf -lIex -lHalf -lIlmThread -lz -ljpeg -ltiff -lpng -lz -lz -lexiv2 -llcms2 ../../celeste/libceleste.so.0.0 -lflann -lflann_cpp ../../hugin_base/libhuginbase.so.0.0 -lX11 -lGL -lGLU -lsqlite3 -lpano13 ../../foreign/levmar/libhuginlevmar.a -lGLEW -lboost_filesystem -lboost_system -lfftw3 -lvigraimpex -lImath -lIlmImf -lIex -lHalf -lIlmThread -lz -ljpeg -lpng -lz -ljpeg -lpng -ltiff -lexiv2 -llcms2 -pthread
/nix/store/scs241fp7dlrvm45d2fjbpjvygirn0ml-binutils-2.35.1/bin/ld: CMakeFiles/cpfind.dir/PanoDetectorLogic.cpp.o: undefined reference to symbol 'LZ4_compress_HC_continue'
/nix/store/scs241fp7dlrvm45d2fjbpjvygirn0ml-binutils-2.35.1/bin/ld: /nix/store/3nf85zf44vyvwr32hx53v7swljxysyjm-lz4-1.9.3/lib/liblz4.so.1: error adding symbols: DSO missing from command line

I shall continue to investigate.

jspricke · 2021-07-07T21:01:31Z

@mikepurvis
are you sure your flann.pc is correct?
What is the output of pkg-config --libs flann?
Your log contains -lflann -lflann_cpp which should be from pkg-config if I read the FindFLANN.cmake correct.

pkgs/development/libraries/flann/default.nix

mikepurvis · 2021-07-08T02:23:42Z

are you sure your flann.pc is correct?

Seems reasonable, I think:

$ cat result/lib/pkgconfig/flann.pc
# This file was generated by CMake for flann
prefix=/nix/store/xrnvfm5f6f9k5lkca0ik3s9z52d84iw3-flann-1.9.1
exec_prefix=${prefix}
libdir=${prefix}/lib
includedir=${prefix}/include

Name: flann
Description: Fast Library for Approximate Nearest Neighbors
Version: 1.9.1
Requires:
Libs: -L${libdir} -L/nix/store/3nf85zf44vyvwr32hx53v7swljxysyjm-lz4-1.9.3/lib;-llz4 -lflann -lflann_cpp
Cflags: -I${includedir}

And this config also works fine with the PCL 1.11 build.

Your log contains -lflann -lflann_cpp which should be from pkg-config if I read the FindFLANN.cmake correct.

Yeah, I would have thought so too, and yet:

$ pkg-config --libs flann
-L/nix/store/xrnvfm5f6f9k5lkca0ik3s9z52d84iw3-flann-1.9.1/lib -L/nix/store/3nf85zf44vyvwr32hx53v7swljxysyjm-lz4-1.9.3/lib\;-llz4 -lflann -lflann_cpp

jspricke · 2021-07-08T12:29:05Z

I had a look, the difference is:
Debian:

$ pkg-config --libs-only-l flann
-llz4 -lflann -lflann_cpp

NixOS

pkg-config --libs-only-l flann
-lflann -lflann_cpp

the NixOS flann.pc

$ cat .nix-profile/lib/pkgconfig/flann.pc
# This file was generated by CMake for flann
prefix=/nix/store/jrpxk67fnda2laqlsij8mwc8g62kp0yk-flann-1.9.1
exec_prefix=${prefix}
libdir=${prefix}/lib
includedir=${prefix}/include

Name: flann
Description: Fast Library for Approximate Nearest Neighbors
Version: 1.9.1
Requires:
Libs: -L${libdir} -L/nix/store/3nf85zf44vyvwr32hx53v7swljxysyjm-lz4-1.9.3/lib;-llz4 -lflann -lflann_cpp
Cflags: -I${includedir}

The problem is the semicolon, which is coming from @LZ4_STATIC_LDFLAGS@ which should be converted to string, I guess. Can you work on a patch?

mikepurvis · 2021-07-08T13:57:29Z

Yes, I can confirm that removing the semicolon causes it to succeed when linking manually. I think this only works for PCL because its version of FindFLANN.cmake doesn't pass through the pkgconfig outputs directly, but rather just uses them as hints to a bunch of find_path/find_library calls.

In any case, yes this should be fairly easy to patch up.

pkgs/development/libraries/flann/default.nix

This change adds two additional patches from Debian's version of libflann, which most critically fix an issue where the lz4 library was being bundled with flann, causing header conflicts when later packages would independently pull flann and another lz4. While I'm here, I've also disabled the unused examples and tests, as well as the MatLab bindings, and placed the Python bindings behind a flag (am open to the default being changed to true for this).

mikepurvis · 2021-07-09T17:16:31Z

Have pushed a new version which I believe addresses all the feedback, and am locally re-running nixpkgs-review pr 129491, will post the results of that when available, but I believe Hugin is now fixed based on a source build.

Regarding the Python story, it turns out it's pretty goofy; it isn't bindings at all, but rather just a handful of uninstalled files that make up a ctypes wrapper:

$ find result/ | grep py
result/share/flann/python
result/share/flann/python/setup.py
result/share/flann/python/pyflann
result/share/flann/python/pyflann/index.py
result/share/flann/python/pyflann/__init__.py
result/share/flann/python/pyflann/exceptions.py
result/share/flann/python/pyflann/flann_ctypes.py

So I've left this is in for now, but it won't be much use to anyone unless the packaging is further updated to actually python3 setup.py install these.

mikepurvis · 2021-07-09T17:38:19Z

I'm not sure what the review-shell thing is all about here, but everything passed dependency rebuild, so I'm going to tick that off the list.

$ nixpkgs-review pr 129491
$ git -c fetch.prune=false fetch --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/129491/head:refs/nixpkgs-review/1
$ git worktree add /home/ciadmin/.cache/nixpkgs-review/pr-129491-2/nixpkgs df0d321258a9b35c36cd9d8688985502d464105f
Preparing worktree (detached HEAD df0d321258a)
Updating files: 100% (26957/26957), done.
HEAD is now at df0d321258a Merge pull request #123765 from hyperfekt/update-bcachefs
$ nix-env --option system x86_64-linux -f /home/ciadmin/.cache/nixpkgs-review/pr-129491-2/nixpkgs -qaP --xml --out-path --show-trace
$ git merge --no-commit --no-ff 3fc8ddab5dd5653e9b6cebe78ed84ca87daceea5
Automatic merge went well; stopped before committing as requested
$ nix-env --option system x86_64-linux -f /home/ciadmin/.cache/nixpkgs-review/pr-129491-2/nixpkgs -qaP --xml --out-path --show-trace --meta
6 packages updated:
cloudcompare digikam digikam flann hugin pcl

$ nix --experimental-features nix-command build --no-link --keep-going --option build-use-sandbox relaxed -f /home/ciadmin/.cache/nixpkgs-review/pr-129491-2/build.nix
error: builder for '/nix/store/lpnx7yw3d21dk37alsl7xqpgm28dhj63-review-shell.drv' failed with exit code 1;
       last 2 log lines:
       > qtPreHook
       > Error: wrapQtAppsHook is not used, and dontWrapQtApps is not set.
       For full logs, run 'nix log /nix/store/lpnx7yw3d21dk37alsl7xqpgm28dhj63-review-shell.drv'.

Link to currently reviewing PR:
https://github.com/NixOS/nixpkgs/pull/129491

5 packages built:
cloudcompare digikam flann hugin pcl

pkgs/development/libraries/flann/default.nix

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

mikepurvis · 2021-07-13T15:38:21Z

Awesome, thanks for the merge!

ofborg bot requested a review from viric July 7, 2021 02:06

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Jul 7, 2021

mikepurvis force-pushed the flann-1.9.1-refactor branch from 9d758bd to 4da9729 Compare July 7, 2021 02:07

ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 11-100 labels Jul 7, 2021

SuperSandro2000 requested changes Jul 8, 2021

View reviewed changes

SuperSandro2000 reviewed Jul 9, 2021

View reviewed changes

pkgs/development/libraries/flann/default.nix Outdated Show resolved Hide resolved

pkgs/development/libraries/flann/default.nix Outdated Show resolved Hide resolved

mikepurvis mentioned this pull request Jul 9, 2021

Use string(JOIN) to avoid semicolons in pc file. flann-lib/flann#480

Open

Review feedback, extra lz4 patch.

0590eb0

mikepurvis force-pushed the flann-1.9.1-refactor branch from 93a1d02 to 3fc8dda Compare July 9, 2021 16:32

Add back python3 dep.

bc8249d

mikepurvis force-pushed the flann-1.9.1-refactor branch from 3fc8dda to bc8249d Compare July 9, 2021 17:14

mikepurvis requested a review from SuperSandro2000 July 9, 2021 17:23

SuperSandro2000 reviewed Jul 9, 2021

View reviewed changes

pkgs/development/libraries/flann/default.nix Outdated Show resolved Hide resolved

Use rec instead of let.

44471cd

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

mikepurvis requested a review from SuperSandro2000 July 11, 2021 01:59

SuperSandro2000 changed the title ~~Flann 1.9.1 refactor~~ flann: refactor Jul 13, 2021

SuperSandro2000 merged commit 64f7cb5 into NixOS:master Jul 13, 2021

mikepurvis deleted the flann-1.9.1-refactor branch July 13, 2021 15:38

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flann: refactor #129491

flann: refactor #129491

mikepurvis commented Jul 7, 2021 •

edited

r-rmcgibbo commented Jul 7, 2021 •

edited

mikepurvis commented Jul 7, 2021

jspricke commented Jul 7, 2021

mikepurvis commented Jul 7, 2021 •

edited

jspricke commented Jul 7, 2021

mikepurvis commented Jul 8, 2021

jspricke commented Jul 8, 2021

mikepurvis commented Jul 8, 2021 •

edited

mikepurvis commented Jul 9, 2021 •

edited

mikepurvis commented Jul 9, 2021

mikepurvis commented Jul 13, 2021

flann: refactor #129491

flann: refactor #129491

Conversation

mikepurvis commented Jul 7, 2021 • edited

Motivation for this change

Things done

r-rmcgibbo commented Jul 7, 2021 • edited

mikepurvis commented Jul 7, 2021

jspricke commented Jul 7, 2021

mikepurvis commented Jul 7, 2021 • edited

jspricke commented Jul 7, 2021

mikepurvis commented Jul 8, 2021

jspricke commented Jul 8, 2021

mikepurvis commented Jul 8, 2021 • edited

mikepurvis commented Jul 9, 2021 • edited

mikepurvis commented Jul 9, 2021

mikepurvis commented Jul 13, 2021

mikepurvis commented Jul 7, 2021 •

edited

r-rmcgibbo commented Jul 7, 2021 •

edited

mikepurvis commented Jul 7, 2021 •

edited

mikepurvis commented Jul 8, 2021 •

edited

mikepurvis commented Jul 9, 2021 •

edited