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

ceres-solver 2.0.0 #63596

Closed

Conversation

NikolausDemmel
Copy link
Contributor

Created with brew bump-formula-pr.

@BrewTestBot BrewTestBot added the missing license Formula has a missing license which should be added label Oct 28, 2020
@NikolausDemmel NikolausDemmel marked this pull request as draft October 28, 2020 11:33
@chenrui333 chenrui333 added the test failure CI fails while running the test-do block label Oct 31, 2020
@dtrodrigues
Copy link
Member

opencv, opencv@3, and rawtoaces need revision bumps.

Should cmake instead be a build-only dependency?

@NikolausDemmel
Copy link
Contributor Author

This also needs some more changes to the formula due to changes in how ceres handles dependencies. I have locally had success to build the ceres formula, but I'm stuck with the dependent formulas:

opencv, opencv@3, and rawtoaces need revision bumps.

Yes. These don't build with the latest release ceres, in fact. They might need small changes in cmake. But the bigger issue is that ceres 2.0.0 now requires C++14, and opencv 4 is built in C++11 mode, opencv@3 in C++99 mode. It is possible to build these formulas with C++14 with small changes, but it's untested and it might affect users (there may be binary incompatibility between c++99 and c++11, I guess).

I see the following alternatives:

  • Wait for upstream to become compatible with ceres 2.0. For opencv 3 and 4 I would guess it will never happen, b/c of C++11. I guess it will take forever until they are phased out. For rawtoaces we would have to see (didn't investigate yet if someone raised this upstream with rawtoaces yet).
  • Remove the formula dependencies, in particular from the opencv formulas. In the current release it's only an optional dependency of the sfm contrib (and a known issue w.r.t. ceres 2.0). Not sure how many would be affected by not having these optional parts of the sfm contrib available from the opencv formula. Moreover, it looks like ceres recently got added additionally as a dependency to the rgbd contrib, but w.r.t. to this issue they said that the best "fix" would be to remove the ceres dependency alltogether from the rgbd contrib.
  • Introduce a versioned formula ceres-solver@1, but after reading the documentation on that, I'm not sure if it qualifies in terms of popularity stats. Moreover, I don't think the ceres maintainers have plans to maintain the 1.14 branch separately (but version 1.14. was released in March 2018, so it has been stable for a long time).

Any suggestions which way to go here?

Should cmake instead be a build-only dependency?

I think so.

@BrewTestBot BrewTestBot added automerge-skip `brew pr-automerge` will skip this pull request and removed automerge-skip `brew pr-automerge` will skip this pull request missing license Formula has a missing license which should be added labels Oct 31, 2020
@jonchang
Copy link
Contributor

jonchang commented Nov 2, 2020

Wait for upstream to become compatible with ceres 2.0. For opencv 3 and 4 I would guess it will never happen, b/c of C++11. I guess it will take forever until they are phased out.

I'd say that this justifies including an ceres-solver@1 formula, given that it is a dependency of the quite popular opencv which (correct me if I'm wrong) is still being maintained. Presumably the opencv maintainers would fix any problems in their own repo if there were issues with ceres-solver 1.14.

Copy link

@alalek alalek left a comment

Choose a reason for hiding this comment

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

This is no issue to bump C++ version for OpenCV binaries if platforms used with Homebrew support that (it is compiler limitation only).
Bumping C++ version in user applications is not required, but we need to ensure "std" ABI compatibility during C++98 => C++11 change on HomeBrew supported platforms (std::string/std::vector/std::shared_ptr are used in OpenCV ABI).

@@ -120,7 +123,7 @@ def install
return 0;
}
EOS
system ENV.cxx, "-std=c++11", "test.cpp", "-I#{include}/opencv4",
system ENV.cxx, "-std=c++14", "test.cpp", "-I#{include}/opencv4",
Copy link

Choose a reason for hiding this comment

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

No need to bump C++ version for User applications.
It is not necessary with modern compilers (last known incompatibility was with GCC 5.x, C++98/C++11 and _GLIBCXX_USE_CXX11_ABI macro).

(BTW, this test.cpp doesn't use OpenCV binaries, like .so/.dylib)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense, thanks. With your upstream changes in opencv I probably don't have to change anything about the formula here.

@BrewTestBot
Copy link
Member

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@BrewTestBot BrewTestBot added stale No recent activity and removed stale No recent activity labels Nov 26, 2020
@NikolausDemmel
Copy link
Contributor Author

NikolausDemmel commented Nov 27, 2020

I'd say that this justifies including an ceres-solver@1 formula, given that it is a dependency of the quite popular opencv which (correct me if I'm wrong) is still being maintained. Presumably the opencv maintainers would fix any problems in their own repo if there were issues with ceres-solver 1.14.

Sorry, I've been very busy at work lately. Thanks for you assessment, that is great, but even better would be if we can actually fix the depending formulas to use ceres 2.0. It looks much more likely than I originally thought:

There was some movement at opencv to make the contrib modules using ceres compatible with version 2.0. I'm not sure when the next release will be though. Possibly in a few months. In the meantime we could consider including the patch in homebrew already, if we wanna move forward with ceres 2.0. (Would have to check if it applies cleanly on the last releases open opencv).

I'll ping the rawtoaces author about ceres 2.0 compatibility.

Update: AcademySoftwareFoundation/rawtoaces#121

@SeekingMeaning
Copy link
Contributor

SeekingMeaning commented Dec 14, 2020

If rawtoaces still isn't compatible when the new opencv releases come out, I think we should disable (or deprecate) it with :does_not_build as the reason. The latest (and only) release was released 3 years ago, and there hasn't been much activity in the GitHub repository other than two PR merges that modified their README.

@jonchang
Copy link
Contributor

Yeah, I have a pull request open with them since august that hasn't been looked at. So probably we should just disable it.

@SeekingMeaning
Copy link
Contributor

opencv - #67482
opencv@3 - #67515

@SeekingMeaning SeekingMeaning dismissed a stale review via 05941f2 January 6, 2021 19:22
@NikolausDemmel NikolausDemmel force-pushed the bump-ceres-solver-2.0.0 branch 2 times, most recently from da2d273 to d438ffd Compare January 7, 2021 10:59
@NikolausDemmel NikolausDemmel marked this pull request as ready for review January 7, 2021 11:00
@NikolausDemmel
Copy link
Contributor Author

Ok, with the upstream changes in opencv no more formula changes seem to be required. Any C++14 requirements seem to be propagated by cmake for the corresponding code parts that depend on ceres. @SeekingMeaning disabled rawtoaces (there hasn't been any reaction upstream) and I squashed the commits, so I think it's ready for review.

@dtrodrigues dtrodrigues removed the test failure CI fails while running the test-do block label Jan 8, 2021
@SeekingMeaning
Copy link
Contributor

% brew linkage ceres-solver
System libraries:
  /System/Library/Frameworks/Accelerate.framework/Versions/A/Accelerate
  /usr/lib/libSystem.B.dylib
  /usr/lib/libc++.1.dylib
Homebrew libraries:
  /usr/local/opt/gflags/lib/libgflags.2.2.dylib (gflags)
  /usr/local/opt/glog/lib/libglog.0.dylib (glog)
  /usr/local/opt/metis/lib/libmetis.dylib (metis)
  /usr/local/opt/openblas/lib/libopenblas.0.dylib (openblas)
  /usr/local/opt/suite-sparse/lib/libamd.2.dylib (suite-sparse)
  /usr/local/opt/suite-sparse/lib/libcamd.2.dylib (suite-sparse)
  /usr/local/opt/suite-sparse/lib/libccolamd.2.dylib (suite-sparse)
  /usr/local/opt/suite-sparse/lib/libcholmod.3.dylib (suite-sparse)
  /usr/local/opt/suite-sparse/lib/libcolamd.2.dylib (suite-sparse)
  /usr/local/opt/suite-sparse/lib/libcxsparse.3.dylib (suite-sparse)
  /usr/local/opt/suite-sparse/lib/libspqr.2.dylib (suite-sparse)
  /usr/local/opt/suite-sparse/lib/libsuitesparseconfig.5.dylib (suite-sparse)
  /usr/local/opt/tbb/lib/libtbb.dylib (tbb)
Indirect dependencies with linkage:
  metis
  openblas
  tbb
Dependencies with no linkage:
  eigen

@SeekingMeaning SeekingMeaning dismissed a stale review via 1468255 January 8, 2021 18:54
@BrewTestBot BrewTestBot removed the automerge-skip `brew pr-automerge` will skip this pull request label Jan 8, 2021
@SeekingMeaning
Copy link
Contributor

SeekingMeaning commented Jan 8, 2021

-DTBB=OFF was added in 7f34377 to avoid opportunistic linkage, and suite-sparse at the time did not depend on tbb (This is no longer the case as of 5195792)

@NikolausDemmel
Copy link
Contributor Author

@SeekingMeaning: Oh, sorry, I didn't realize you wanted the commits split by file. Thanks for fixing it.

-DTBB=OFF was added in 7f34377 to avoid opportunistic linkage, and suite-sparse at the time did not depend on tbb (This is no longer the case as of 5195792)

TBB was a direct dependency in ceres 1.14, but they removed support for it and now it is only polled in b/c it may be used indirectly by suite-sparse. The TBB cmake argument was removed in ceres.

http://ceres-solver.org/installation.html

@SeekingMeaning
Copy link
Contributor

Yes, sorry for the misunderstanding—my comment was meant more for record-keeping (so we can refer back to this PR and see why this specific change was made)

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@NikolausDemmel NikolausDemmel deleted the bump-ceres-solver-2.0.0 branch January 9, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants