iliana / rust-crowbar

Wrapper to simplify writing AWS Lambda functions in Rust (using the Python execution environment)
https://docs.rs/crowbar
Apache License 2.0
197 stars 16 forks source link

(experimental) re-export cpython items #46

Open softprops opened 5 years ago

softprops commented 5 years ago

This is an experimental change that will addresses https://github.com/ilianaw/rust-crowbar/issues/45. If this checks out users of rust crowbar will no longer have to declare cpython as a dependency in their Cargo.toml of lib.rs files.

softprops commented 5 years ago

I'm going to do a bit more integration testing but I wanted to iterate early to show what this could look like

softprops commented 5 years ago

I was able to get a working POC in https://github.com/softprops/serverless-crowbar/pull/3/files. all that was needed was to update the python3-sys feature use.

I would say something like that wouldn't be needed at all if that were declared directly in rust crowbar as lando does I wouldn't see that as a bad thing given we can't change reality of the python3.6 lambda runtime. If nothing it would make for a better default experience for crowbar users, like myself :)

softprops commented 5 years ago

@ilianaw, I feel less rushed with this change knowing that 0.3 was just published. I got antsy because I saw 0.4 milestone items were closing out and it looked like there was some release prep for that. I'd be able to do a bit more testing on this but let me know what you think.

iliana commented 5 years ago

The crowbar 0.3.0 milestone is still open, it's cpython-json 0.3.0 that was published :)

In my experience reexporting crates back when I first developed this was difficult. This method was probably just as possible then, but maybe I was hoping for first-class macro reexporting to exist.

This feels like a hack (anything involving #[doc(hidden)] feels like one to me) but I think the developer experience is worth it. I'm going to give it some more thought after a bit but I expect I'll merge this as-is. More testing would be much appreciated though :)

softprops commented 5 years ago

I'll try testing more cases. I'm open to alternate approaches. Coming from the jvm world of transitive dependencies re-exporting in rust does feel a bit off. I can see where it makes sense through when considering macros. Macros are just code that generates code. If the crowbar macro generates code that expands to another crates macros then it's understandable to need to add the other macro use. It's just an odd ux for users because that's just an impl detail that bleeds through. Reexportingbis a tactic for making that detail #[hidden] :)

softprops commented 5 years ago

Good news here. Macro reexports just landed on stable https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1300-2018-10-25 this is just what this crate needs

iliana commented 5 years ago

I'm definitely fine with requiring Rust 1.30 for 0.3.0 for macro re-exports :)

softprops commented 5 years ago

I'm going to experiment with this a bit. So far not having much luck. After moving to #[macro_export(local_inner_macros)] I see

---- src/lib.rs -  (line 25) stdout ----
error[E0433]: failed to resolve. Could not find `py_module_initializer` in `$crate`
 --> src/lib.rs:29:1
  |
5 | / lambda!(|event, context| {
6 | |     println!("hi cloudwatch logs, this is {}", context.function_name());
7 | |     // return the event without doing anything with it
8 | |     Ok(event)
9 | | });
  | |___^ Could not find `py_module_initializer` in `$crate`
softprops commented 5 years ago

I've got some direction from these to be published docs https://github.com/rust-lang-nursery/edition-guide/pull/117/files#diff-4e57182b623c416bc83329f6a7307ec9R177. The foreign macros crowbar uses needed the cpython:: path prefix. I'm going to experiment with this a bit more and report back.

softprops commented 5 years ago

Oof! that worked but using the lambda macro then requires the user to import all of the helper macros that cpython uses inside its macros. I'm going to see if I can't reach out to the cpython crates author about making this a bit more convenient for its users.

I added the following one by one until I got crowbars doc tests to work

use cpython::{
  py_argparse_parse_plist,
  py_argparse_parse_plist_impl,
  py_fn_impl, py_method_def,
  py_argparse_raw,
  py_argparse_impl,
  py_argparse_extract,
  py_replace_expr,
  py_argparse_param_description
};
softprops commented 5 years ago

wanted to leave an update on this as I'm still keeping an eye on the cpython issue. still no progress towards using the 1.30.0 stable form of $crate::/local_inner_macros as an alternative to re-exporting. I'm not sure how active that project is so I'm not sure how long to sit on the upstream solution. I could maybe circle back and see if there's an alternative way.

iliana commented 5 years ago

In terms of upstream, see #19 and... some possibly exciting news: https://github.com/PyO3/pyo3/issues/210 — looks like PyO3 is willing to part with specialization for now in order to build on stable.

softprops commented 5 years ago

Nice. That may end out being a safer bet. I'm getting the impression cpython crate is not very actively maintained. That makes it harder for crates like this to evolve :/ Have you started experimenting with pyo3 yet?

iliana commented 5 years ago

Have you started experimenting with pyo3 yet?

Not at all :(

softprops commented 5 years ago

Got some feed back from the cpython crew that is not actively maintained any more :/ which means it may be behooving to start casting a closer look at pyo3. I'm about to go on vacation so I wont be as active as I have been in recent weeks. If I get time I can start poking a bit at that. I'm thinking that's also going to need some kind of serde adapter like the one you use for crowbar. I see one jsonlike thing listed under pyo3's tooling section I can take a peek.

iliana commented 5 years ago

nod

Enjoy your vacation :)

iliana commented 5 years ago

Dropping the 0.3.0 milestone while we figure out a path forward here.