Skip to content
This repository has been archived by the owner on Jun 13, 2019. It is now read-only.

(experimental) re-export cpython items #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

softprops
Copy link
Contributor

This is an experimental change that will addresses #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
Copy link
Contributor Author

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

@softprops
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Owner

iliana commented Oct 3, 2018

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 :)

@iliana iliana added this to the 0.3.0 milestone Oct 3, 2018
@softprops
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

iliana commented Oct 26, 2018

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

@softprops
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

softprops commented Oct 26, 2018

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
Copy link
Contributor Author

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
Copy link
Owner

iliana commented Oct 31, 2018

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

@softprops
Copy link
Contributor Author

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
Copy link
Owner

iliana commented Oct 31, 2018

Have you started experimenting with pyo3 yet?

Not at all :(

@softprops
Copy link
Contributor Author

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
Copy link
Owner

iliana commented Nov 2, 2018

nod

Enjoy your vacation :)

@iliana iliana removed this from the 0.3.0 milestone Nov 16, 2018
@iliana
Copy link
Owner

iliana commented Nov 16, 2018

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants