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

openmvg: init at v1.1 #25480

Merged
merged 1 commit into from
May 5, 2017
Merged

openmvg: init at v1.1 #25480

merged 1 commit into from
May 5, 2017

Conversation

mdaiter
Copy link
Contributor

@mdaiter mdaiter commented May 3, 2017

Motivation for this change

Let's add OpenMVG to the NixOS ecosystem!
OpenMVG is a package for performing Structure-from-Motion on sets of images.
Currently, OpenMVG is a hassle to compile on NixOS; therefore, we should make an easy-to-use derivation for compiling this piece of software on NixOS.

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 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.

hardeningDisable = [ "all" ];

meta = {
description = "A library for computer-vision scientists and targeted fort the Multiple View Geometry community";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typo here with "for"(t). I'll remove this in another PR to avoid an entire recompile of OpenMVG on a single thread (don't want to exhaust your virtual memory on the testing machine).

@mdaiter
Copy link
Contributor Author

mdaiter commented May 5, 2017

@LnL7 or another community member, could I please get a review? Shouldn't be too bad -- just some C++ compiling ;)

@@ -0,0 +1,46 @@
{ lib, stdenv, fetchgit, pkgconfig, cmake, gnumake, glibc
, enableJpeg ? false, libjpeg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to add the flags instead of simply passing libjpeg = null; to callPackage (directly or via override)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how it's done in OpenCV, which has a similar build system and also relies on boolean conditions to determine appropriate CMake flags. While those CMake flags aren't included in this initial push, I'm making a PR to include them. If you'd like, I can include them in this PR (I'll squash it)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, you can even check (libjpeg == null) to determine the flags.

OpenCV has a few flags not literally mapped to dependency libraries (enableContrib, enableIpp)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@7c6f434c , I am also including options as to whether to build documentation or examples as we speak, both of which are not literally mapped to dependency libraries. I'd like to keep the naming congruent and uniform, but if you think it'd be better as a non-uniform naming schema, please let me know and I can change it!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, if you do need CMake flags (as opposed to just passing null instead of some buildInputs) and there are also documentation/examples flags, then this makes more sense.

Your PR definitely looked like you wanted to use the expression as-is, if you have immediate plans to update the expression, comment on the PR after pushing everything (GitHub notifications about force-pushes to PRs work in some weird way).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@7c6f434c apologies. Finalised expression is currently posted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@7c6f434c should gnumake also be added to the build inputs? Can gnumake build on Darwin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not a problem, just explaining the reference point for my criticism.

};

buildInputs =
[ glibc ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it actually break if you do not pass glibc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. Haven't tried it, although I generally prefer to be as atomic as possible and assume as little as possible about the current build system at hand. Will the compile break if glibc (or another appropriate POSIX-provider) is not passed in? As in, will NixOS inherently include this if stdenv is already included?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Providing a libc is the job of stdenv; explicit passing of glibc is likely to make porting the package to darwin (macOS) platform harder (unless just removing glibc is enough, of course…).

};

buildInputs =
[ cmake ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be in nativeBuildInputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. My bad (generally get nativeBuildInputs and buildInputs confused.

++ lib.optional enableTiff libtiff
;

nativeBuildInputs = [ gnumake pkgconfig ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gnumake is not needed — stdenv takes care of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, removed it. This should also enable builds on Darwin with the new updates. Going to add in support and see if the Darwin build successfully completes.


configurePhase = ''
cmake ./src -DCMAKE_INSTALL_PREFIX=$out -DCMAKE_BUILD_TYPE=Release $cmakeFlags
'';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you do here is a slightly downgraded version of:

cmakeDir = "./src";
dontUseCmakeBuildDir = true;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the dontUseCmakeBuildDir?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, your code doesn't create build and cd to it… I think the current version of setupHook for cmake doesn't handle cmakeDir correctly when using build directory.

You could also try just setting setSourceRoot='' sourceRoot="$(echo */src/)"; ''; and let cmake support in NixPkgs do its job.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair. I'll do the original suggestion now.

Copy link
Contributor Author

@mdaiter mdaiter May 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also, will those cmakeFlags be appended to the cmake command automatically?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Nix will pass cmakeFlags from the expression into the environment of the builder, and the builder reads cmakeFlags from the environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even that $out flag? Seems to not be expanding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, these flags are not needed at all, because they are used by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh great!

"-DOpenMVG_BUILD_EXAMPLES=${if enableExamples then "ON" else "OFF"}"
"-DOpenMVG_BUILD_DOC=${if enableDocs then "ON" else "OFF"}"
"-DCMAKE_CXX_FLAGS=-std=c++11"
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, you don't eventually generate anything based on dependency status, why use flags and not just allow passing null? That would not require any special support (except a comment that these dependencies are optional)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the general coding style. If you want, I can change these to null parameters

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, adding five parameters and later handling them just to do the same as a single comment would accomplish seems too verbose to me.

# This can be enabled, but it will exhause virtual memory on most machines.
enableParallelBuilding = false;

hardeningDisable = [ "all" ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any comments why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without hardeningDisable, certain flags are passed to the compile that break the build (primarily string format errors)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would comment in the expression that the problems are related to that.

url = "https://www.github.com/openmvg/openmvg.git";

# Tag v1.1
rev = "f5ecb48";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know you can use explicit tags here? i.e. rev = "v${version}";

Copy link
Contributor Author

@mdaiter mdaiter May 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rev = "refs/tags/v${version}"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For reference, I've pushed an updated version that uses this)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, you need submodules and use fetchgit not fetchFromGithub

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

, zlib ? null
, libpng ? null
, eigen ? null
, libtiff ? null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do note that callPackage does pass flags that have default value (i.e. the default is currently to use all the optional dependencies, not to use the minimal set). General NixPkgs decision making on optional dependencies is based on «how often extra features are useful» vs. «total closure size change», and include-everything is quite a typical outcome (no change required here, just giving some context)

@7c6f434c 7c6f434c merged commit 587ed89 into NixOS:master May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants