Skip to content

Fix cephes static linking#2273

Merged
dellaert merged 3 commits intoborglab:developfrom
Gold856:fix-cephes-static-linking
Oct 19, 2025
Merged

Fix cephes static linking#2273
dellaert merged 3 commits intoborglab:developfrom
Gold856:fix-cephes-static-linking

Conversation

@Gold856
Copy link
Copy Markdown
Contributor

@Gold856 Gold856 commented Oct 7, 2025

Same fix I applied to METIS. Apparently the cephes header also contained declaration for C math functions? Removed those because we have those functions in the standard library.

The header is broken. I prefer to do this rather than maintain a duplicate header
@talregev
Copy link
Copy Markdown
Contributor

I like the way you handle cephes. but Remove C math function declarations should be in different PR that we can discuss and see.
What happen if you revert this commit in this PR?

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Oct 10, 2025

It fails with a inconsistent dll linkage on those functions, because MSVC's standard libraries also export those functions: https://github.com/Gold856/gtsam/actions/runs/18264608447/job/51997148050#step:8:640

We should include math.h or cmath if we want to use those functions instead.

@talregev
Copy link
Copy Markdown
Contributor

https://github.com/Gold856/gtsam/actions/runs/18264608447/job/51997148050#step:8:640

Can you show me the changes of this run? It look like static build errors.

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Oct 10, 2025

The full branch is https://github.com/Gold856/gtsam/commits/windows-static, with that run being from the commit before I removed the C math function declarations. It also has the /permissive- changes so it can build up to that point.

@talregev
Copy link
Copy Markdown
Contributor

I think we need to solve one problem of the time. clearly you want to try to solve the static compilation.
I think we need first to handle only the export of cephes first, that will help the direction you want also -> static compilation.
and discuss gtsam static compilation in different PR.

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Oct 11, 2025

I showed you my static build branch instead of this PR is because you'll only notice the difference in static builds, which are the target of this PR. The additional changes are just so that it doesn't fail on other unrelated stuff. Yes, this includes the /permissive- changes because static linking is broken in other ways, as I showed in that PR.

I believe all the commits in this PR are both independent and small enough that they can both be reviewed independently and another PR is separate. The split would essentially be modifying the exports (to fix static linking) and removing the C math function declarations (to fix static linking.) That functionally doesn't really gain much, especially since removing C math function declarations shouldn't impact usage, as cephes is included by a header explicitly marked as internal. I think it makes more sense to keep it as part of one PR.

@ProfFan
Copy link
Copy Markdown
Collaborator

ProfFan commented Oct 13, 2025

What is cephes used for here? Only for Chi square right? If so, can we just replace the lib/inline it?

@talregev
Copy link
Copy Markdown
Contributor

I also have another suggestion.
Compile cephes as c++ and add a namespace

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Oct 13, 2025

Yeah, it's only used for ChiSquaredInverse. This apparently isn't used anywhere except for testGncOptimizer? Someone else is going to have to clarify the purpose of this function existing, because I'd vote to just outright remove cephes and this function.

As for inlining it, I tried to remove as much of cephes as possible, and got it down to 13 files:
image
We could consolidate it and put it into one big file that gets compiled into gtsam. We could compile it as C++ and provide a namespace, since there's a unit test for it, but I want to see if we can remove cephes outright.

@varunagrawal
Copy link
Copy Markdown
Contributor

The GncOptimizer class relies on ChiSquaredInverse to work. When we removed Boost as a dependency, GncOptimizer stopped compiling since it was using the ChiSquaredInverse provided by Boost.

I added Cephes since it was an easy and lightweight replacement and Luca Carlone was keen on having GncOptimizer working again (he spoke to me about this in person). There was no other implementation available and implementing the inverse gamma functions is far too much work.

If @ProfFan or any of you want to take it up, please feel free. I spent weeks trying to implement it without much success (especially with the full unit testing etc).

@dellaert
Copy link
Copy Markdown
Member

I'm not a big fan of cephes either, and would welcome a local implementation of ChiSquaredInverse. But, heed @varunagrawal 's words :-)

Varun, is there a PR for that or a branch?

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Oct 14, 2025

Ah, it's used in setInlierCostThresholdsAtProbability. I missed that my first time around. Then I think this PR is fine as-is. I certainly do not have the math knowledge to reimplement ChiSquaredInverse, but I can simplify down our copy of cephes to the core 13 files we need for igami if we want.

@talregev
Copy link
Copy Markdown
Contributor

better not changing the original files.
We can change only the header that export the symbols for the actual functions that gtsam is using.

@varunagrawal
Copy link
Copy Markdown
Contributor

Varun, is there a PR for that or a branch?

For a local version of ChiSquaredInverse? I think I gave up and got rid of whatever hacky version I had (which was nowhere near completion). This was also almost 2 years ago. :(

Copy link
Copy Markdown
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

LGTM - but I know not enough. Ready to merge then?

@Gold856
Copy link
Copy Markdown
Contributor Author

Gold856 commented Oct 19, 2025

Go for it!

@dellaert dellaert merged commit 45c475b into borglab:develop Oct 19, 2025
32 checks passed
@Gold856 Gold856 deleted the fix-cephes-static-linking branch October 19, 2025 17:39
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.

5 participants