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

feat: Expose pact_matching and pact_models over FFI. #97

Merged
merged 37 commits into from
Apr 23, 2021

Conversation

alilleybrinker
Copy link
Contributor

This is a fairly large PR, so I'll break it down.

This PR introduces a new crate, called pact_matching_ffi, which exposes the internals of pact_matching and some from pact_models for use by other languages over an FFI.

The design and decisions are documented in the ARCHITECTURE.md file. Specifically, it is a non-invasive FFI design, meaning it involves no changes to the contents of the crates it's exposing. The other design decisions stem from this.

The intent of the introduction of this crate is to make it easier to build other language implementations of Pact by having them call the reference implementation under the hood, something my team has already begun doing.

It's built with CMake (a relatively recent version), and includes finder modules for Cargo and cbindgen. It does require a nightly Cargo version to run cbindgen, because the FFI-exposed functions are defined inside of a macro which can only be expanded for use by cbindgen using an unstable flag on rustc. The CMakeLists.txt file checks that a nightly version of Cargo is being used, as without this check an attempt to run the cbindgen step of the CMake build process would error out after a substantial time with an inscrutable error message.

Happy to talk through the design decisions here. We've tried to document the safety and error handling concerns as thoroughly as possible.

This work was done by @cstepanian and @alilleybrinker (me).

Notice

This software was produced for the U. S. Government under Contract No. FA8702-20-C-0001, and is subject to the Rights in Noncommercial Computer Software and Noncommercial Computer Software Documentation Clause DFARS 252.227-7014 (FEB 2014)

Approved for Public Release; Distribution Unlimited. Case Number 19-3203.

(c) 2021 The MITRE Corporation. All Rights Reserved.

alilleybrinker and others added 30 commits April 22, 2021 09:50
This adds a brand new crate called "pact_matching_ffi" to the pact-reference/rust workspace.
This commit adds an error handling mechanism modelled after the
mechanism in `libgit2`, where errors (and panics, to avoid undefined
behavior by panicking across a language boundary) are captured and
written as messages to a `LAST_ERROR` value in thread-local storage.
Functions which error out in this way are expected to return some value
which indicates an error (a status code or a null pointer, commonly),
leaving the C code to get the last error message and (potentially)
handle the error.

This commit also adds in some utility functions, some of which are
needed for future additions (including basic pointer operations and an
FFI convenience macro) and some of which are needed currently
(specifically a safe mechanism for converting a Rust `String` into
a `CString` and writing it to a C buffer).

Another thing to note is that all FFI functions are expected to be
wrapped in the FFI-helper which ensures errors are captured and
reported properly, with the exception of the function to retrieve the
latest error itself. As such, the error-handling API must be
particularly scrutinized now and in the future to ensure it doesn't
leave open the possibility of cross-language panics.
* Added logging infrastructure.

Added mechanism for C callers to configure log sinks and set up the
logger, and for the FFI code to log function names, parameter values,
and return values.

* Changed rustfmt config and reran rustfmt
This allows for the creation and deletion of a Message. Messages may be
added as a default message or from a JSON source.
Adds CMake to both power Cargo compilation and generation of header files with cbindgen, and enable easy use of the FFI from other languages with a more accessible build system.
This commit adds a number of Message APIs to the FFI, including
message provider state and message metadata APIs.
Bump pact_matching version in FFI and fixed some Clippy lints.
Improvements to existing message implementation to better fit agreed-on
patterns and consistently follow design requirements. Not yet fully
finished, but good enough to merge.
This commit adds the ability to get and iterate over metadata for Pact messages.
Adds ability to read provider state data for a Message.
* Update Doxygen configuration in CMake

* Make Doxygen generate docs properly

This commit does three things:

* Removes a `.gitkeep` file we do longer need (because the dir has a
  `.gitignore` too)
* Modifies `cbindgen.toml` to put a header at the top of the generated
  file which tells Doxygen to process it correctly.
* Modifies `CMakeLists.txt` to always rerun the header-generator, and
  further modifies Doxygen configuration as we need, plus some slight
  style improvement to the Doxygen section of the config.
Unintentionally broke the build with my rebase, as `pact_matching_ffi`
currently locks the `pact_matching` version number, and it had changed
upstream. In the future, the Pact project may prefer to leave off the
version number, but for us it makes things more predictable.
This worked on a Windows system because NTFS ignores
case differences.
Also add methods to read the names contained in Consumer
and Provider.
* New FFI macro

This commit replaces the previous FFI macro with a much cleaner, more
user-friendly one, which retains all the benefits of the prior one
while adding some new benefits, including inserting a Clippy silencer
that was otherwise quite annoying when trying to use `anyhow` in our
FFI context.

* Applied new FFI macro to message_pact

* Removed unnecessary error code returns

* Added message_get_contents

* Added message_pact_find_metadata

* Updated message_pact consumer and provider getters to new macro

* Corrected consumer and provider macros

* Fixed compilation
First pass at basic matching with the FFI.

This commit introduces the first matching function of the FFI,
`match_message`, along with (and more importantly), a mechanism for
iterating over and inspecting mismatches which are returned by this
and other future matching methods.
* Make cbindgen ignore certain internals

* Added missing documentation
This allows client code to avoid importing the helper macro
'cstr' if only 'safe_str' is used.

The absolute paths should also protect against namespace collisions.
@alilleybrinker alilleybrinker changed the title Expose pact_matching and pact_models over FFI. feat: Expose pact_matching and pact_models over FFI. Apr 22, 2021
@uglyog
Copy link
Member

uglyog commented Apr 23, 2021

Awesome PR, the patterns will be useful in the other FFI crates as well

@uglyog uglyog merged commit 7c3b36b into pact-foundation:master Apr 23, 2021
@alilleybrinker alilleybrinker deleted the pact_matching_ffi branch April 23, 2021 13:54
@uglyog
Copy link
Member

uglyog commented Apr 25, 2021

@alilleybrinker I'm having some trouble getting the pact_matching_ffi crate building on CI (GH Actions). I don't have much CMake knowledge, so if someone could review that changes it would be appreciated.

The CMake files needed to be modified in the following manor:

  • CMake build fails if doxygen is not present (i.e. on Windows and OSX agents). I was able to put a conditional around it.
  • In PactMatchingFfiConfig.cmake.in, set(IMPORT_NAME "") for non-windows causes set_target_properties to fail. I put another conditional around that.
  • The CMake build expects output lib files to be named pact_matching_ffi.* but the Rust build generates files with lib prefixed (i.e. libpact_matching_ffi.a)

One issue I could not fix was cbindgen was not generating synthetic structs for the Consumer and Provider Rust struts, so the example projects fail to build. I could not work out why, but those structs are not really required, because they are just used to get the name via a method, so we could just pass in the MessagePact class here instead.

I.e. change

    MessagePact *message_pact = message_pact_new_from_json(file_name, json_str);
     if (message_pact == NULLPTR) {
         // handle error.
     }
    
     Consumer *consumer = message_pact_get_consumer(message_pact);
     if (consumer == NULLPTR) {
         // handle error.
     }
    
     char *name = consumer_get_name(consumer);
     if (name == NULL) {
         // handle error.
     }

to

    MessagePact *message_pact = message_pact_new_from_json(file_name, json_str);
     if (message_pact == NULLPTR) {
         // handle error.
     }
    
     char *name = message_pact_get_consumer_name(message_pact);
     if (name == NULL) {
         // handle error.
     }

@alilleybrinker
Copy link
Contributor Author

@uglyog we're happy to take a look! I'll hand this off to @cstepanian who was the other contributor to this new crate.

The issue of not generating struct names for Consumer and Provider is interesting. I think it may be because of this line in the cbindgen.toml file, which specifies the dependent crates to process for possible FFI-exposure. Even though pact_matching_ffi re-exports from pact_models, without pact_models also being shown here the symbols likely aren't getting output, as you've observed. This was an oversight on my part.

I think with fixing this that the other API change you show here won't be necessary.

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