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

FFI Sync Engine changes #223

Merged
merged 7 commits into from
May 25, 2024
Merged

FFI Sync Engine changes #223

merged 7 commits into from
May 25, 2024

Conversation

abrassel
Copy link
Contributor

Change list

Add sync engine cfg option to the ffi. This will allow us to build with local file paths only (mostly useful for testing).

How is this tested?

Changed ffi-test.c to build both a sync and default engine. It should pass both.

Note This is a draft because I can't get the test to build right. For some reason, the codegenned c code isn't defining the SYNC-ENGINE flag. Any ideas @nicklan

Issue

This addresses #199

@abrassel
Copy link
Contributor Author

@scovich do you happen to know why the C define is missing in the codegen for SYNC-ENGINE?

ffi/cffi-test.c Outdated Show resolved Hide resolved
@scovich
Copy link
Collaborator

scovich commented May 24, 2024

@scovich do you happen to know why the C define is missing in the codegen for SYNC-ENGINE?

I believe cbindgen tries to emit a single header that works no matter which features were selected (and uses #if defined checks to strip out unneeded bits of code if a specific feature isn't enabled). But cbindgen has no way to guess which features the downstream user actually enabled, so it's up to the library user to pass an appropriate -D to the compiler. In case of cffi-test, the Makefile currently sets the default engine:

CFLAGS=-c -Wall -DDEFINE_DEFAULT_ENGINE #`pkg-config --cflags arrow-glib`

Most likely you just need to add -DDEFINE_SYNC_ENGINE there and all will be well?

ffi/cffi-test.c Outdated Show resolved Hide resolved
ffi/src/lib.rs Outdated Show resolved Hide resolved
ffi/cffi-test.c Outdated Show resolved Hide resolved
ffi/src/lib.rs Outdated Show resolved Hide resolved
@abrassel abrassel marked this pull request as ready for review May 24, 2024 17:01
@abrassel abrassel requested a review from scovich May 24, 2024 17:05
@abrassel
Copy link
Contributor Author

@scovich ready for re-review!

Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

nice thanks. just a couple small things

ffi/cffi-test.c Outdated Show resolved Hide resolved
ffi/src/lib.rs Outdated Show resolved Hide resolved
ffi/src/lib.rs Outdated Show resolved Hide resolved
ffi/cffi-test.c Outdated Show resolved Hide resolved
@abrassel abrassel requested a review from nicklan May 24, 2024 17:42
Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

one last small thing please!

ffi/src/lib.rs Outdated Show resolved Hide resolved
@abrassel abrassel requested a review from nicklan May 24, 2024 17:59
ffi/src/lib.rs Outdated Show resolved Hide resolved
@abrassel abrassel requested a review from nicklan May 24, 2024 19:57
Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

thanks for iterating! lgtm!

Copy link
Collaborator

@scovich scovich left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

@abrassel
Copy link
Contributor Author

@nicklan @scovich can one of y'all merge this!

@nicklan nicklan merged commit 2d913ec into delta-io:main May 25, 2024
9 checks passed
@nicklan
Copy link
Collaborator

nicklan commented May 25, 2024

Merged! Thanks for the contribution!

@abrassel abrassel deleted the ffi-sync-engine branch May 29, 2024 05:47
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.

3 participants