-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[openexr,openimageio,suitesparse,theia] updates for non-win32 #6371
Conversation
Hi @cenit, could you also bump the CONTROL version in openimageio port? I noticed that this port didn't update the version in CONTROL file. |
x64-windows-static had no regressions
x86-windows had no regressions
x64-uwp had no regressions arm-uwp had no regressions |
I found out that clapack was not really working before, at least on windows: FindLAPACK from cmake failed on it and so every port using it was not properly initialized. Also suitesparse didn't find it and so was built without, and ceres[suitesparse] was silently failing (no real suitesparse feature enabled, failing gracefully to a simple ceres). |
@@ -0,0 +1,46 @@ | |||
include(${CMAKE_ROOT}/Modules/SelectLibraryConfigurations.cmake) |
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.
This looks like it should just be a -config.cmake
file, not using our vcpkg-cmake-wrapper.cmake
"hack".
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.
The problem is that a -config.cmake file has lower priority than an integrated module, while our hack has higher. I’d agree with you but if I do a config then cmake will happily forget it, except when dev is explicitly requesting find_package(LAPACK CONFIG) [or NO_MODULE], which no one does
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.
I see. In that case, it would be better to "split" this into two parts:
- This content should be in a
FindLAPACK.cmake
file installed intoshare/clapack
- We should use
vcpkg-cmake-wrapper.cmake
to temporarily append this directory to the module path, then run the user's originalfind_package()
command. This retains the user's ability to deploy their own find modules or config files.
# vcpkg-cmake-wrapper.cmake
set(LAPACK_PREV_MODULE_PATH ${CMAKE_MODULE_PATH})
list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR})
_find_package(${ARGS})
set(CMAKE_MODULE_PATH ${LAPACK_PREV_MODULE_PATH})
This helps to ensure we get the full, proper find_package()
context so that the various functions work correctly.
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.
I tried to implement your suggestions. Please let me know what do you think
New regressions 😞
|
Exactly what I feared. No one was really using clapack... but I have to open logs later (on the phone now) to understand failures of clapack itself. I tried it on few architectures and it was working 🤔 |
ok I found some errors which should fix many problems. I hope for the best! |
@vicroms please give me some nice news... |
@vicroms I can see from public CI that x64-linux has failures for openmvg and sophus. I can't reproduce them locally, but I can't see logs from public CI, so I am lost. |
Waiting for CI results. |
I just saw that public CI was lamenting a failed thombstone even if I bumped the control just to trigger a rebuild... now I modified port file a little bit to see if it kicks it harder... |
@vicroms thanks, modifying port file now it tried a rebuild of sophus but failed. I am unable to see how to download logs and also can't reproduce it locally, so I need your help |
Hi @cenit Glad to help! I see a green check for this PR on Linux, which would explain why no logs were published. Looks like a failed I also noticed that the public Windows CI is missing the Analyze Results job in the latest build. So it is also missing logs. For now, we'll have to wait for build 2721208 in the private CI to finish and get the logs. You may already know them, but the important things to review in CI builds are:
NOTE: These may change as @Rastaban keeps working on the public CI. |
ok thanks. |
Green mark? 😃 |
Results are legit, here are the affected packages:
|
Too many fixed ports? 🤣 |
Thank you for your contribution. : ) |
may require #6364 for proper build results
patches ported from #5169