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

use the cbindgen crate and some decorators to DRY out the rust/python ffi #6869

Merged
merged 5 commits into from Dec 7, 2018

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Dec 5, 2018

Problem

Our FFI between pants python and rust is currently split across:

  1. @ffi.def_extern()-decorated methods in native.py,
  2. some C code in literal strings in that file,
  3. numerous #[repr(C)] and #[no_mangle] annotations sprinkled around the rust codebase.

Unless you use a regex-based code search tool, this can make it difficult to understand how to change the FFI. There is duplication of the interface definition in the above ways, which can potentially be a source of subtle-to-not-so-subtle errors.

Solution

  • Introduce the cbindgen crate to generate all the requisite C header declarations for native.py from our Rust sources automagically (I was very impressed).
    • There was some hackery required with re.sub(), noted in two new methods named _hackily_*.
  • Create _ExternSignature class and @_extern_decl annotation to mark python methods with cffi equivalents.
  • Move FFI initialization to a new _FFISpecification class which injects a self._ffi object and has a method to generate FFI declarations for methods marked with @_extern_decl, as well as register them on the injected self._ffi object.

Result

Changes to Rust objects and methods on the FFI interface no longer require changing multiple places at once, and the type declarations for FFI methods from python are generated from an @_extern_decl annotation at the method definition itself. cbindgen can and will e.g. add const where necessary, so offloading the duty to the cbindgen crate (which is also being used in Servo/WebRender, so I'm not too concerned about maintainability) gives us more than just reducing boilerplate, it also improves correctness.

@cosmicexplorer cosmicexplorer requested review from stuhood and illicitonion Dec 5, 2018

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pants-bootstrap-cffi branch from ae20675 to 0c1977a Dec 5, 2018

@stuhood

stuhood approved these changes Dec 5, 2018

Copy link
Member

stuhood left a comment

This looks really great. Nice work!

let crate_dir = env::var("CARGO_MANIFEST_DIR").unwrap();
cbindgen::generate(crate_dir)
.expect("Unable to generate bindings")
.write_to_file(scheduler_file_path);

This comment has been minimized.

@stuhood

stuhood Dec 5, 2018

Member

Does this method return a Result? Not sure whether build scripts render warnings.

If it doesn't return a Result, then it looks like we're unwrap()ing everything in here, and there is no need for this main to return Result?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 5, 2018

Contributor

I think the issue was that when I tried to use the ? operator, it didn't have the right failure type for the return value of main() -> std::io::Result<()>. I definitely would like to remove .unwrap() calls like the env var one here given that we can wrap it all in a Result and will look to see if that works (and if a top-level Result renders failures).

This comment has been minimized.

@stuhood

stuhood Dec 5, 2018

Member

Understood. But you do not currently use the ? operator.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 7, 2018

Contributor

I have added some Froms and removed all the .unwrap()s except for path strings, let me know if I should convert those too. I have also added a comment noting that build scripts will render errors to stderr.

@@ -0,0 +1,17 @@
header = '''
/* TODO: add header guards and license info! */

This comment has been minimized.

@stuhood

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 7, 2018

Contributor

Done!

return wrapper


class FFISpecification(object):

This comment has been minimized.

@stuhood

stuhood Dec 5, 2018

Member

Could classes that are not meant for consumption outside this file be prefixed with _? _FFISpecification and etc.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 7, 2018

Contributor

Yes! Will do.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 7, 2018

Contributor

Done!

cosmicexplorer added some commits Dec 5, 2018

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:pants-bootstrap-cffi branch from 0c1977a to 893d6a8 Dec 7, 2018

cosmicexplorer added some commits Dec 7, 2018

void lease_files_in_graph(Scheduler*);
void garbage_collect_store(Scheduler*);

This comment has been minimized.

@wisechengyi

wisechengyi Dec 7, 2018

Contributor

Q: Using garbage_collect_store as an example, is it needed to have its corresponding changes, or is it a feature of the new lib as well?

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 7, 2018

Contributor

So the usage of the cbindgen crate will produce this file src/rust/engine/src/cffi/scheduler.h when pants is bootstrapped from this repo -- in this case, that generated header includes the line:

void garbage_collect_store(Scheduler *scheduler_ptr);

So the idea is that we can hopefully rely on cbindgen to do this for us, or better. It is good that the result (the generated scheduler.h header) is compatible with cffi -- I expect this to continue to be true, but any new dependency has risks.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Dec 7, 2018

Contributor

Well, actually, the generated header already isn't immediately compatible with cffi, and we are already doing some regex substitution hacks to get around this -- and any further updates to cffi would be to support more of normal header files such as #include lines, I think, not less.

@cosmicexplorer cosmicexplorer merged commit f4cab2d into pantsbuild:master Dec 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment