Refactor runtime crates into feature-guarded modules #131

jcrossley3 · 2021-06-17T22:18:23Z

To address #130

slinkydeveloper · 2021-06-21T07:16:01Z

Ok the first thing I would do is to split this PR into several PRs, one per submodule. Also don't remove the old packages now, we can do that later.

slinkydeveloper · 2021-06-21T07:16:12Z

You also need to sign the DCO :)

src/lib.rs

src/actix/server_request.rs

jcrossley3 · 2021-06-21T12:53:54Z

You also need to sign the DCO :)

I signed all but the one I need help with. I can do that when I figure out how to fix it.

jcrossley3 · 2021-06-21T13:05:53Z

Ok the first thing I would do is to split this PR into several PRs, one per submodule. Also don't remove the old packages now, we can do that later.

So you prefer 1-PR-per-submodule to 1-commit-per-submodule as I currently have it? I can do that, but I'm curious why that's better? Once I back out the deletion of the existing packages and FromRequest impl, we'll only be left with the 4 submodule commits in this PR. Is that better than 4 PR's with one commit each?

jcrossley3 · 2021-06-21T13:58:27Z

Ok, I just force-pushed changes that leave the old workspace dirs intact and don't add the FromRequest impl. Just one commit for each submodule feature. I changed the ce- feature prefix to cloudevents-, and addressed the test/lint failures as well.

Because tests are replicated in the parent workspace now, each will run twice for cargo test --workspace --all-features

slinkydeveloper

Can you try to run locally cargo doc --open --all-features to see if this breaks the doc build? You should also try cargo publish --dry-run to see if any publish check breaks (although I'm not sure you can do that without logging in)

src/actix/mod.rs

slinkydeveloper · 2021-06-23T15:03:45Z

src/actix/headers.rs

+use std::collections::HashMap;
+use std::str::FromStr;
+
+macro_rules! unwrap_optional_header {


Not for this PR, but maybe for future refactorings: a lot of these macros are copy paste, because the APIs are practically the same even if their names are different, so it would be nice to find a way to unify them

Addressed in #146

src/actix/mod.rs

slinkydeveloper · 2021-06-23T15:14:37Z

Another thing I'm not 100% sure of is how this is gonna play out with our existing CI config, with our matrix of different targets... For example, the actix feature is not supposed to build with wasm. I might be wrong, but it seems to me that the integrations are not tested anymore with this PR, without modifying the existing github actions config.

cloudevents-sdk-actix-web/Cargo.toml

jcrossley3 · 2021-06-23T19:39:12Z

Can you try to run locally cargo doc --open --all-features to see if this breaks the doc build? You should also try cargo publish --dry-run to see if any publish check breaks (although I'm not sure you can do that without logging in)

Done. Fixed several broken links. I only tested the publish from the root and it seemed fine, abandoning only because of --dry-run and without requiring me to log in.

jcrossley3 · 2021-06-23T20:03:39Z

Another thing I'm not 100% sure of is how this is gonna play out with our existing CI config, with our matrix of different targets... For example, the actix feature is not supposed to build with wasm. I might be wrong, but it seems to me that the integrations are not tested anymore with this PR, without modifying the existing github actions config.

I added an --all-features to every step that had --workspace. This essentially runs all the tests twice. When we delete the workspace dirs, we can remove the --workspace option in the actions, too.

As far as the wasm32 build goes, I added the cloudevents-reqwest feature to the args, but I'm unable to see this working locally. I can't get past the following:

[jim@localhost reqwest-wasm-example]$ npm run serve

> @ serve /home/jim/src/rust/sdk-rust/example-projects/reqwest-wasm-example
> webpack-dev-server

🧐  Checking for wasm-pack...

ℹ️  Installing wasm-pack 

ℹ ｢wds｣: Project is running at http://localhost:8080/
ℹ ｢wds｣: webpack output is served from /
ℹ ｢wds｣: Content not from webpack is served from /home/jim/src/rust/sdk-rust/example-projects/reqwest-wasm-example
✖ ｢wdm｣: Error: Rust compilation.
    at ChildProcess.<anonymous> (/home/jim/src/rust/sdk-rust/example-projects/reqwest-wasm-example/node_modules/@wasm-tool/wasm-pack-plugin/plugin.js:210:16)
    at ChildProcess.emit (events.js:376:20)
    at maybeClose (internal/child_process.js:1055:16)
    at Socket.<anonymous> (internal/child_process.js:441:11)
    at Socket.emit (events.js:376:20)
    at Pipe.<anonymous> (net.js:673:12)

slinkydeveloper · 2021-06-25T13:51:08Z

As far as the wasm32 build goes, I added the cloudevents-reqwest feature to the args, but I'm unable to see this working locally. I can't get past the following:

Mhhh that error is weird, perhaps the js deps needs to be updated

Conditionally compile actix module when enabled Mostly straightforward, though I don't particularly love that dev-deps can't be optional: rust-lang/cargo#1596 Signed-off-by: Jim Crossley <[email protected]>

Conditionally compile reqwest module when enabled This resulted in a naming conflict between my desired feature name, "reqwest", and the optional dependency itself. So I adopted the convention of prefixing the features with "cloudevents-". Signed-off-by: Jim Crossley <[email protected]>

Conditionally compile rdkafka module when enabled Signed-off-by: Jim Crossley <[email protected]>

Conditionally compile warp module when enabled Signed-off-by: Jim Crossley <[email protected]>

Verified `cargo doc --all-features` works Signed-off-by: Jim Crossley <[email protected]>

For the wasm32 build, we replace --package <<redundant>> with --features cloudevents-reqwest Signed-off-by: Jim Crossley <[email protected]>

jcrossley3 · 2021-06-25T14:35:32Z

As far as the wasm32 build goes, I added the cloudevents-reqwest feature to the args, but I'm unable to see this working locally. I can't get past the following:

Mhhh that error is weird, perhaps the js deps needs to be updated

Didn't help. I get the same error on master, so I don't think this PR broke it. Besides, all the checks are passing. ;)

I think I've addressed all your concerns. Let me know if you need anything else!

Now that we've refactored the protocol bindings from crates to feature-guarded modules (PR cloudevents#131), we can remove the workspaces for those crates. Signed-off-by: Jim Crossley <[email protected]>

Now that we've refactored the protocol bindings from crates to feature-guarded modules (PR #131), we can remove the workspaces for those crates. Signed-off-by: Jim Crossley <[email protected]>

jcrossley3 force-pushed the single-crate branch from 353604c to 68afb1c Compare June 17, 2021 22:27

jcrossley3 marked this pull request as ready for review June 18, 2021 02:00

slinkydeveloper reviewed Jun 21, 2021

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/actix/server_request.rs Outdated Show resolved Hide resolved

jcrossley3 force-pushed the single-crate branch from c94c451 to 958dcb3 Compare June 21, 2021 13:54

jcrossley3 force-pushed the single-crate branch from 958dcb3 to 78b298b Compare June 21, 2021 14:23

jcrossley3 changed the title ~~[WIP] Refactor runtime crates into feature-guarded modules~~ Refactor runtime crates into feature-guarded modules Jun 21, 2021

jcrossley3 force-pushed the single-crate branch from 78b298b to 46f6015 Compare June 21, 2021 16:26

slinkydeveloper reviewed Jun 23, 2021

View reviewed changes

cloudevents-sdk-actix-web/Cargo.toml Outdated Show resolved Hide resolved

jcrossley3 force-pushed the single-crate branch from ba3e399 to ebee809 Compare June 23, 2021 20:01

jcrossley3 added 6 commits June 25, 2021 10:12

New feature: cloudevents-actix

935234a

Conditionally compile actix module when enabled Mostly straightforward, though I don't particularly love that dev-deps can't be optional: rust-lang/cargo#1596 Signed-off-by: Jim Crossley <[email protected]>

New feature: cloudevents-rdkafka

9055d71

Conditionally compile rdkafka module when enabled Signed-off-by: Jim Crossley <[email protected]>

New feature: cloudevents-warp

538b647

Conditionally compile warp module when enabled Signed-off-by: Jim Crossley <[email protected]>

Fixed docs

69f16a5

Verified `cargo doc --all-features` works Signed-off-by: Jim Crossley <[email protected]>

Build/Test --all-features for action matrix

4979c44

For the wasm32 build, we replace --package <<redundant>> with --features cloudevents-reqwest Signed-off-by: Jim Crossley <[email protected]>

jcrossley3 force-pushed the single-crate branch from ebee809 to 4979c44 Compare June 25, 2021 14:16

slinkydeveloper added the enhancement New feature or request label Jun 25, 2021

slinkydeveloper added this to the 0.4 milestone Jun 25, 2021

slinkydeveloper merged commit 362492f into cloudevents:master Jun 25, 2021

jcrossley3 mentioned this pull request Jun 28, 2021

De-workspace-ification #135

Merged

jcrossley3 deleted the single-crate branch July 7, 2021 21:37

Refactor runtime crates into feature-guarded modules #131

Refactor runtime crates into feature-guarded modules #131

Uh oh!

Conversation

jcrossley3 commented Jun 17, 2021

Uh oh!

slinkydeveloper commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slinkydeveloper commented Jun 21, 2021

Uh oh!

Uh oh!

Uh oh!

jcrossley3 commented Jun 21, 2021

Uh oh!

jcrossley3 commented Jun 21, 2021

Uh oh!

jcrossley3 commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slinkydeveloper left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slinkydeveloper Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

jcrossley3 Jul 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

slinkydeveloper commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jcrossley3 commented Jun 23, 2021

Uh oh!

jcrossley3 commented Jun 23, 2021

Uh oh!

slinkydeveloper commented Jun 25, 2021

Uh oh!

jcrossley3 commented Jun 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

slinkydeveloper commented Jun 21, 2021 •

edited

Loading

jcrossley3 commented Jun 21, 2021 •

edited

Loading

slinkydeveloper commented Jun 23, 2021 •

edited

Loading