-
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
[ceres + downstreams] Update to ceres to 2.2.0 #42475
base: master
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
Build failures appear to be unrelated to this change? |
They look legitimate to me? A couple of examples: Direct failures:
Indirect failures, for instance:
are downstream broken components: PS D:\vcpkg> .\vcpkg.exe depend-info cartographer 2>&1 | findstr ceres
ceres[lapack, suitesparse]: eigen3, glog, lapack, suitesparse, vcpkg-cmake, vcpkg-cmake-config
cartographer: boost-iostreams, cairo, ceres, gflags, glog, gtest, lua, protobuf, vcpkg-cmake, vcpkg-cmake-config
PS D:\vcpkg> .\vcpkg.exe depend-info openturns 2>&1 | findstr ceres
ceres: eigen3, glog, vcpkg-cmake, vcpkg-cmake-config
openturns: blas, boost-multiprecision, boost-random, ceres, cminpack, dlib, exprtk, hdf5, lapack, libxml2, mpc, mpfr, muparser, nlopt, pagmo2, pthread, python3, spectra, tbb, vcpkg-cmake, vcpkg-cmake-config
PS D:\vcpkg> .\vcpkg.exe depend-info openmvg 2>&1 | findstr ceres
ceres[cxsparse, lapack, suitesparse]: eigen3, glog, lapack, suitesparse, vcpkg-cmake, vcpkg-cmake-config
openmvg: cereal, ceres, coin-or-clp, coin-or-osi, coinutils, eigen3, flann, libjpeg-turbo, liblemon, libpng, tiff, vcpkg-cmake, vcpkg-cmake-config, vlfeat, zlib
PS D:\vcpkg> |
Whene I tested the usage of
Cofig:
|
@JonLiu1993 Thanks for finding the issue. This should also be fixed now. |
What matters is the total size of the patches, not how many files it's split into; I agree separating them would be bad.
Hmmmm.... they also have very high port-versions suggesting that they are painful. Perhaps we should deindex them. |
That's not what I meant to say. To clarify: what I meant is that I only added a relatively small delta of new changes to the existing patches. The main change I made was to reapply them to the updated underlying sources and combined them into fewer patch files. The only project with somewhat significant changes is rtabmap, for which I already upstreamed the changes in a PR. It will take time for these to be consumable here though. I would argue that, if we were fine accepting these patches in the past, then we should be fine again now. It would be unfortunate if we delay the update of ceres much longer. Many external projects rely on it and would greatly benefit from this update. I still recommend decommissioning the theia and cartographer ports, given that the upstream projects are not maintained anymore. |
I guess, what I mean is, I don't think we should have accepted a 1500 line patch in the first place. That's well outside of our normal patch guidelines, and it's unwarranted if there is no universe where upstream could someday accept it.
Yes, those are the two in question here. @AugP @ras0219-msft @JavierMatosD @vicroms and I discussed today and agree these should be removed from the index. I'll push that into this PR shortly. |
… patches to adapt to updated ceres versions, and are unmaintained by upstreams.
I deindexed cartographer entirely and have an action item to update the maintainer guide about these cases where large patches are necessary to get unmaintained downstream dependents functional. Do you have other concerns here @JonLiu1993 ? @ahojnnes Are you happy with this 'the ports got deindexed' outcome? |
Tested usage successfully by
|
Co-authored-by: Kai Pastor <[email protected]>
Thank you, sounds good to me! |
Looks there is a error in x64-linux, this is error log: |
@JonLiu1993 Shouldn't have removed the openmvg:x64-linux from the ci exclusions :-) I'll see if I can fix it, but otherwise will revert back to the previous state and skip it. |
f4e9c34
to
4301643
Compare
@JonLiu1993 @BillyONeal All checks should be green now. Thank you for your reviews and support. |
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.
LGTM, please fix the file conflicts
# search should be for an exact match, but for usability reasons do a soft | ||
# match and reject with an explanation below. | ||
-find_package(Eigen3 ${CERES_EIGEN_VERSION} QUIET) | ||
+find_package(Eigen3 CONFIG ${CERES_EIGEN_VERSION} QUIET) |
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.
find_package calls should never be QUIET in vcpkg
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.
If you look at the next few lines in the original config file, you will find that it is not really quiet but it fails if the find_package call was unsuccessful. It has some custom logic that alters internal variables. I suggest keeping the patch minimal, as the behavior will be the same.
…/jsch/ceres-2.2.0
./vcpkg x-add-version --all
and committing the result.