Add OpenFHE library #7869
Add OpenFHE library #7869
Conversation
…y wider platform support
|
Ready for review and & merge 🙏 @giordano |
There was a problem hiding this comment.
Can this patch be upstreamed? It doesn't look specific to Yggdrasil.
There was a problem hiding this comment.
Unfortunately, I failed to reproduce the error on my Macbook. I thus figured it is maybe a specific combination of Xcode and/or Clang. But I will try to get it upstreamed and amend the build recipe here accordingly once (if) a new, fixed upstream version is available.
It would be great if we could still get this PR merged already now, to allow us to continue working on further downstream packages.
There was a problem hiding this comment.
It's not a matter of being able to reproduce the error, but of correctness: if the standard declares that a function is defined in a certain header file, the header file should always be included, regardless of whether some toolchains happen to accidentally define said function through other includes. And my understanding is that abs is supposed to be defined by cmath: https://cplusplus.com/reference/cmath/
There was a problem hiding this comment.
There was a problem hiding this comment.
It's not a matter of being able to reproduce the error, but of correctness: if the standard declares that a function is defined in a certain header file, the header file should always be included, regardless of whether some toolchains happen to accidentally define said function through other includes. And my understanding is that
absis supposed to be defined bycmath: https://cplusplus.com/reference/cmath/
Sorry, just saw your reply now. Totally agree. I've created an upstream PR. It would still be great though if we can proceed here and I create a new release once the upstream fix has been published.
There was a problem hiding this comment.
Note though that the problematic abs here is the one for int arguments (see error message in openfheorg/openfhe-development#633). Rereading the references, cmath only defines the float versions of abs, while cstdlib seems to be indeed the source for the int version (and is included in the header in question). However, including cstdlib does not prevent the error either, so I'm waiting to see if the upstream folks have a take on this 🤷♂️
There was a problem hiding this comment.
Note though that the problematic abs here is the one for int arguments
Doesn't look like it. The error message emanating from line 118 in that header is trying to do std::abs on a double value since the code is
inline bool is64BitOverflow(double d) {
return std::abs(d) > static_cast<double>(Max64BitValue());
}
so it needs cmath to have that abs. (and similarly for the error from line 137, which is also doing an abs of a double).
There was a problem hiding this comment.
Thanks for your insights. I'll pass this on to the upstream devs should it come up.
* Add OpenFHE * Add CSL to provide libgomp; disable unittests/benchmarks for hopefully wider platform support * Disable MUSL builds since it is not supported * Use suitable dependencies for OpenMP support * Exclude unsupported architectures (PowerPC & 32-bit x86) * Add patch for macOS * Add missing "
* Add OpenFHE * Add CSL to provide libgomp; disable unittests/benchmarks for hopefully wider platform support * Disable MUSL builds since it is not supported * Use suitable dependencies for OpenMP support * Exclude unsupported architectures (PowerPC & 32-bit x86) * Add patch for macOS * Add missing "
Build JLL package for OpenFHE.