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(macro): Adds custom derive attributes #150

Merged

Conversation

thomastaylor312
Copy link
Contributor

This adds custom derive attributes to the generate macro. It also fixes a bug where package version was not appended to the hash map of exports

Closes #148

@thomastaylor312
Copy link
Contributor Author

@peterhuene This is only draft because right now it is depending on a git dependency of wit-bindgen until 0.13 can be released. That release also combined two of the rust crates (see bytecodealliance/wit-bindgen#683), so I had to make those changes here as well. I did test this manually with some projects I was using, but let me know if I missed anything

Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks great!

I think we should probably add a build test that applies a drive and tests it (like a simple serialization test).

@peterhuene
Copy link
Member

Also, as this depends on an unreleased version of wit-bindgen, I'm going to hold off on merging until #147 is merged so that I can cut the first releases of the crates.

@thomastaylor312
Copy link
Contributor Author

I think we should probably add a build test that applies a drive and tests it (like a simple serialization test).

@peterhuene I didn't add one because there is already a test in wit-bindgen that tests for this. Did you still want to add one here as a smoke test (just making sure it can compile with the additional_derives value set)?

@thomastaylor312
Copy link
Contributor Author

Ok, added a test anyway @peterhuene

@peterhuene
Copy link
Member

@peterhuene I didn't add one because there is already a test in wit-bindgen that tests for this. Did you still want to add one here as a smoke test (just making sure it can compile with the additional_derives value set)?

Yeah, it'd be good to have the test to cover that the argument to cargo_component_bindings::generate! actually works, in addition to the test coverage in wit-bindgen to ensure the generated bindings are correct.

Ok, added a test anyway @peterhuene

Thanks!

@thomastaylor312
Copy link
Contributor Author

thomastaylor312 commented Oct 2, 2023

I'll leave this in draft to avoid accidental merge until either

a) You've cut a version of cargo-component to release the crate
b) wit-bindgen releases a new tagged version

@sehz
Copy link

sehz commented Oct 5, 2023

Can we merge this? We are relying on this feature. Suggest having wit-bindgen as a git dependency since this is not published anyway.

@peterhuene
Copy link
Member

I'm about to publish a 0.2.0 version of cargo-component and 0.1.0 version of the wit tools to crates.io.

When that is finished, we can merge this as-is with the git references, which will block any future release until we get a new wit-bindgen published.

@peterhuene
Copy link
Member

We're now good to rebase this PR and change it from a draft; we don't need to wait for a wit-bindgen release.

@thomastaylor312
Copy link
Contributor Author

Ok, I'll rebase!

This adds custom derive attributes to the generate macro. It also fixes
a bug where package version was not appended to the hash map of exports

Closes bytecodealliance#148

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
@thomastaylor312 thomastaylor312 force-pushed the feat/custom_derive_attr branch from 9bcafa2 to b5b34c8 Compare October 10, 2023 20:56
@thomastaylor312 thomastaylor312 marked this pull request as ready for review October 10, 2023 20:56
@thomastaylor312
Copy link
Contributor Author

@peterhuene This should be good to go

@peterhuene peterhuene merged commit 2c45278 into bytecodealliance:main Oct 11, 2023
6 checks passed
@peterhuene
Copy link
Member

@thomastaylor312 thanks!

@thomastaylor312 thomastaylor312 deleted the feat/custom_derive_attr branch October 11, 2023 20:13
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.

Adding custom derive attributes
3 participants