Open proppy opened 1 month ago
This quoted part seems likely to be problematic:
NOTE: Crubit currently expects deep integration with the build system, and is difficult to deploy to environments dissimilar to Google's monorepo. We do not have our tooling set up to accept external contributions at this time.
The idea of xlsynth-crate is to be seamlessly usable from Cargo, which is why build.rs downloads an appropriate shared library, dymamically loads it, and has knowledge of the symbols at that version via C ABI. C++ automatic binding systems tend to need compiler uniformity and/or build system capabilities, both of which I'm avoiding via the simplicity of a C API. The same principle could go for e.g. making XLS bindings using CFFI in Python -- the simple C ABI guarantees bindings can be made easily from a multitude of languages.
I'm also not sure how Goog binding strategy may change over time, e.g. consider by analogy the challenges CLIF had https://github.com/google/clif -- simple C ABI export is guaranteed to be enduring.
This quoted part seems likely to be problematic:
Indeed, if we were to go w/ crubit, it would be easier to maintain the Rust bindings directly in the XLS tree https://github.com/google/xls/
C++ automatic binding systems tend to need compiler uniformity and/or build system capabilities, both of which I'm avoiding via the simplicity of a C API.
I don't think both approach are incompatible
extern "C"
function binding anyway).even though I agree that (B) wouldn't strictly be needed, it would still allow us to automate the generation of something like https://github.com/xlsynth/xlsynth-crate/blob/main/src/c_api.rs.
wdyt?
I'm not sure I understand the proposal, could you break it down more for me?
Are you saying c_api.rs
would be created in the google/xls
upstream repo and then we could just grab it and release it as part of the libxls.a
release process? (If so, no objections, just don't want to add it to this crate's cargo build process -- not sure if I'm understanding correctly, though.)
Sorry for not being clear, I'm was proposing the following:
Externally:
//xls/public:c_api
cc_library
target in https://github.com/google/xlslibxls.a
and the output of bazel build --config=crubit-genfiles //xls/public:c_api
.build.rs
(or vendor it if appropriate)Internally:
xlsynth-crate
and buildozerize it to depends on the internal source of truth for xls/public:c_api
.Wdyt?
Got it -- so long as crubit is coming from the xls workspace and is added upstream (to google/xls) with some kind of build_test or GH action that ensures it doesn't break I'm good with that, I wouldn't want to figure out how to maintain it "in xlsynth/xlsynth only".
Related to #1, but creating a separate issue since both topic could be discussed separatly.
Did you consider using https://github.com/google/crubit to simplify interfacing w/ XLS's C bindings?