-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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/OpenMVS] fix tools #12229
[OpenMVG/OpenMVS] fix tools #12229
Conversation
The failures for
|
@NancyLi1013 thanks for the review. I was copying tools also when feature was not enabled... |
e53fdfb
to
4ed4059
Compare
I cannot reproduce CI build failures locally. Both x64-windows-static and Linux build fine here. |
I was expecting a "openmvs now passes on CI in x64-linux, please remove it from ci.baseline.txt", this is the opposite 🤣 (also openmvg now fails for CI) |
Converted to draft due to some problems highlighted. Will re-open it as soon as they are fixed |
Errors highlighted that I am fixing are due to problems within the libraries themselves, not due to CI errors. Error in
Simply, this is the first port using both libraries and highlighting that they are build incorrectly. x64-linux too is due to an error in building SuiteSparse with OpenBLAS. The bigger problem is that I don't have those problems locally, both in x64-Linux and in x64-windows-static, so it might be due to binarycaching libraries with problems and not hashing modifications in external files that might have fixed them in the meantime... |
ok both libraries now work when used in a downstream project. Before they were maybe building here on vcpkg, but for sure they were broken when imported downstream... |
@cdcseacave are you interested in accepting most of the fixes for OpenMVS upstream? |
Thank you for making this patches. I understand most of the OpenMVS build changes and I can apply them directly in the main original repo as long as they maintain the compatibility with Linux and MacOS (from your tests it seems to do). Some changes I do not understand though, like those at the end of the main CMakeLists.txt and in MvgMvsPipeline.py. Can you pls explain why are those needed and if I should make them in the main repo as well? |
MvgMvsPipeline.py modifications are characteristic for vcpkg. Since we know perfectly where the tools from openmvg are located, I applied those info to fix relative paths provided and use correct ones. Those shall not be upstreamed, since they might be broken if the user does not adopt a folder strategy identical to vcpkg. I don’t understand exactly otoh which modifications are you referring to when you were asking for details regarding cmake toolchain. |
@NancyLi1013: He should do a |
@Neumann-A |
@cenit: You need to of course update your master first before you try to restore some changes ;) or use :
you currently restored an old version of qt5-base/portfile.cmake |
6ee4b30
to
d29561c
Compare
I removed the unnecessary whitespaces-only changes in qt5-base portfile, which remained after the fix for #12387 was applied externally from this PR and merged with master. Please let me know if you have any other question or review for this PR |
The only thing we need to do is test this PR. |
All features have finished testing. The test results are as follows:
Note: openmp is currently broken on Windows. Has been disabled on Windows.
|
@cenit
Besides these two features, others work fine now. |
OpenMVG software does not work on static CRT build, I know that and fixes are going on to make it work in the future (not for this PR). |
@cenit If this feature is not support static build currentlly, please add an error message to portfile.cmake. |
@JackBoosY @NancyLi1013 I added an error message in OpenMVG in case "software" feature is requested in a CRT-static triplet and I should have fixed the mlpack "tools" feature regression Please let me know, as always, if you have any other review or question |
LGTM now.
This PR can be tagged as |
@NancyLi1013 You don't need to wait for CI to pass before marking reviewed; we aren't going to merge it red :) |
@cenit Thanks for your EXTENSIVE contribution! I don't claim to truly understand everything in here but I don't see any red flags and @strega-nil already signed off. Merged! |
Upgrade SuiteSparse, add new vlfeat port, restore the custom FindLAPACK (for clapack) I wrote one year ago that went lost (and with it each and every port depending on clapack was broken again on linux), remove the unnecessary FindBLAS module, and finally
along the way, it fixes: #12387 (a working qt5 was necessary to build tools cited above) and fixes many problems encountered along the way towards a working solution