-
-
Notifications
You must be signed in to change notification settings - Fork 17.3k
colmap: 3.9.1 -> 3.12.5-unstable-openimageio, poselib: init at 2.0.5 #438672
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
Conversation
36e60b7 to
3ee9e82
Compare
bbbee45 to
470bfa5
Compare
0d206fe to
0ab11b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall look good to me. I reviewed and did not test.
I mark as "Request changes" only because:
- the formatting CI fails; OK to merge from my side once that's fixed
- this needs to target
stagingbecause of10.rebuild-linux: 5001+; probably because of thelibjpeg_turbopatch removal revert
0479796 to
289992c
Compare
289992c to
c2c71eb
Compare
nh2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good but the mega-patch needs some handling. Godo to merged after that in my opinion.
| cudatoolkit | ||
| cudaPackages.cuda_cudart.static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While moving this code and in case you're interested in picking up maintainership of colmap:
cudaPackagages.cudatoolkitis asymlinkJoinof a bunch of other things and we try to avoid itlib.getStatic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a bit confusing to call these pythonDeps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That all sounds good but I'm not sure how that works and it sounds like it would need upstream to switch to FindCudaToolkit. We can do it, but I'd like to get the package unbroken first.
pkgs/by-name/co/colmap/package.nix
Outdated
| ] | ||
| ++ lib.optionals cudaSupport [ | ||
| autoAddDriverRunpath | ||
| ]; | ||
|
|
||
| passthru.updateScript = gitUpdater { }; | ||
| passthru.pythonDeps = pythonDeps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passthru are weird about splicing... I wonder if the better choice would be to just add a dev output and use propagatedBuildInputs (propagatedNativeBuildInputs for cuda_nvcc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(even though, conceptually, I wish Nixpkgs used propagatedBuildInputs less, and passthru more)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SomeoneSerge What exactly do you mean by "weird about splicing"?
In any case, it seems acceptable to me to do it this way for now to unbreak the package; if a different way is cleaner, we can still do that as a follow-up.
deb6417 to
712b6f2
Compare
|
I have decided to take |
f46528f to
68417b2
Compare
68417b2 to
95c4a9f
Compare
This is needed to get unreleased OpenImageIO support so we can drop FreeImage.
95c4a9f to
758b83b
Compare
nh2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My requests regarding the patch were addressed, I verified it, and the package builds and runs. Merging.
|
The ofborg So I'll click
for that, as in the past I have also waited extremely long for Darwin ofborg. If that's wrong, somebody tell me. If the package fails on Darwin on Hydra, we can take it out as a follow-up. |
I'm picking up #390412 by @usertam. This PR additionally upgrades to the unstable OpenImageIO branch.
Outstanding problems:
COLMAP uses FreeImage which is very insecure and abandoned, although they intend to replace it with OpenImageIO (3384). This is in progress: colmap/colmap#3459libjpeg-turbo in nixpkgs recently dropped support for FreeImage (608422bd4ba434d02278602bc74c46d10bfde2ba). This is totally understandable, but unfortunately it means people can't build COLMAP even at their own risk. I've reverted this now, but I'm not sure that's the best thing to do.Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.