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

alice-vision: init at 3.0.0, add 3.1.0 #256115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zeuner
Copy link
Contributor

@zeuner zeuner commented Sep 19, 2023

Description of changes

Opening this since #224262 appears to be stale. @hesiod left off at a very promising state where a build was already configured to work without pulling in submodules. I moved forward up to the point where the package has working (usable for doing actual 3D meshing) binaries and added a package for the more experimental release upstream made in the meantime.

I would consider the packages as a valuable addition since they would move us a significant step forward in achieving #216403

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@zeuner
Copy link
Contributor Author

zeuner commented Sep 19, 2023

@AndersonTorres I had to touch your nanoflann package since alice-vision requires an older version to compile. You might want to review whether you are fine with the changes.


cmakeFlags =
let
cmakeOption = name: enabled: "-D${name}:BOOL=" + (if enabled then "ON" else "OFF");
Copy link
Member

Choose a reason for hiding this comment

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

Is the :BOOL needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the :BOOL omitted, some of the CMake cache entries are created as strings instead of booleans. Not sure whether it can make a practical difference, though. The build configuration doesn't change here.

Copy link
Member

Choose a reason for hiding this comment

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

This is because I am writing a helper function for CMake:

#256418

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 a generic helper function, I would always mandate for setting more exact typing data if it's possible. This way, maintainers could possibly get higher quality metadata for CMake without extra effort, while getting to that point might beconsidered as too tedious at a single package scope.

Copy link
Member

Choose a reason for hiding this comment

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

I have found only one place where those "data types" are defined:

https://cmake.org/cmake/help/latest/command/set.html

It looks too gui-driven, and also too dependent on the upstream - what if this variables are truly strings that happen to act like booleans?

Nonetheless, it should be easy to adjust.

Copy link
Member

Choose a reason for hiding this comment

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

Too off-topic. Solving this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any blockers left?

Copy link
Member

Choose a reason for hiding this comment

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

Not on my part.

@zeuner
Copy link
Contributor Author

zeuner commented Oct 4, 2023

Investigating the failing tests it turned out they happened due to a binary incompatibility with cctag. They are now fixed by disabling libEigen alignments when cctag support is enabled. In order to always use alignment, #258950 would have to be fixed.

At this point, from what I perceive this package is on-par with the monolithic build (8411621).

@infinisil
Copy link
Member

The pkgs/by-name check failure can be safely ignored, it's a problem with the check that's to be fixed with #285089

@infinisil
Copy link
Member

The above PR is now merged and used in CI, so it won't fail anymore the next time you push :)

@infinisil
Copy link
Member

Although I don't think it's great how the expression is effectively duplicated between the two versions. See #285922 (comment) for my recommendation for multiple package versions.

@zeuner
Copy link
Contributor Author

zeuner commented Feb 15, 2024

The above PR is now merged and used in CI, so it won't fail anymore the next time you push :)

Thanks for your work! I need to retrigger CI anyway due to #284145

@zeuner
Copy link
Contributor Author

zeuner commented Feb 15, 2024

Although I don't think it's great how the expression is effectively duplicated between the two versions.

Fully agree. When I started where #224262 left off, I wanted to keep the changes to 3.0.0 minimal considering that this was the code @hesiod initially agreed to maintain. A few months having passed without a reaction, I'm not even sure whether this is still the case. So I'll refactor as I think it will fit.

I still find it useful to have multiple versions since there seem to be 3D reconstruction pipelines that perform worse on newer versions (see alicevision/AliceVision#1501).


alice-vision_3_0_0 = pkgs.alice-vision.overrideAttrs (old: rec {
version = "3.0.0";
src = old.src // {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
src = old.src // {
src = old.src.override {

@pbsds
Copy link
Contributor

pbsds commented Feb 16, 2024

consider marking broken on darwin

@pbsds
Copy link
Contributor

pbsds commented Feb 16, 2024

It also seems the nanoflann override needs more work, as it fails to build when merged with master

@bouk
Copy link
Contributor

bouk commented Feb 23, 2024

FYI there's a fix for the darwin failure in staging: #287255

So I think it's fine to merge with the broken Darwin build

@pbsds
Copy link
Contributor

pbsds commented Feb 25, 2024

#256115 (comment) is still blocking

nix-repl> alice-vision_3_0_0.src    
«derivation /nix/store/icpw6kvspi5ws2rksv2dswkwwwgvngbz-source.drv»

nix-repl> alice-vision.src       
«derivation /nix/store/icpw6kvspi5ws2rksv2dswkwwwgvngbz-source.drv»

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants