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

Remove serde_derive dependency, inlining expanded impls into source code #222

Closed
wants to merge 3 commits into from

Conversation

eqrion
Copy link
Collaborator

@eqrion eqrion commented Oct 15, 2018

This commit removes the dependency on serde_derive so that
cbindgen doesn't depend on a stale package for OS's like debian.

A new file, README.serde_derive is added with instructions on
how to regenerate the impls with changes.

This commit removes the dependency on `serde_derive` so that
cbindgen doesn't depend on a stale package for OS's like debian.

A new file, README.serde_derive is added with instructions on
how to regenerate the impls with changes.
@eqrion
Copy link
Collaborator Author

eqrion commented Oct 15, 2018

r? @emilio

You obviously don't have to review the copy and pasted impl's, I'm more interested in a sanity check.

This will unblock the uploading of cbindgen to Debian and shift the burden of this issue from users to the maintainers.

There might be a CI failure, I didn't run this through rustfmt yet. Hopefully the autogenerated stuff is fine.

@eqrion
Copy link
Collaborator Author

eqrion commented Oct 15, 2018

... and there are rustfmt errors. I'll sprinkle #[cfg_attr(rustfmt, rustfmt_skip)] over the commit and repush.

Copy link

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Be aware that the generated code uses serde private APIs so is not guaranteed to compile or do the right thing against newer serde versions. That's fine within cbindgen as long as the impls are regenerated every time serde is bumped in Cargo.lock, but means that if you want other crates to use cbindgen as a library then you will need to pin a specific serde version in cbindgen's Cargo.toml. Otherwise things will break (frequently).

@eqrion
Copy link
Collaborator Author

eqrion commented Oct 15, 2018

Be aware that the generated code uses serde private APIs so is not guaranteed to compile or do the right thing against newer serde versions. That's fine within cbindgen as long as the impls are regenerated every time serde is bumped in Cargo.lock, but means that if you want other crates to use cbindgen as a library then you will need to pin a specific serde version in cbindgen's Cargo.toml. Otherwise things will break (frequently).

That's an understandable policy, I hadn't thought of that.

Thinking about it further, I'm not sure we can even rely on Cargo.lock for binaries as cargo install cbindgen doesn't use the lock file in my experience.

@eqrion
Copy link
Collaborator Author

eqrion commented Oct 15, 2018

I suppose this change amounts to pinning serde instead of pinning serde_derive.

There is one benefit I could see to this, we can update the pinned version of serde when needed but currently can't with serde_derive. Still not a great solution.

Copy link
Collaborator

@emilio emilio left a comment

Choose a reason for hiding this comment

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

I don't think doing this is worth it. It's going to be a major pain for us and contributors alike...

If we really have to do it (which I think it's debatable if the main issue is 'old serde_derive version can't be backported to beta in Debian'), I'd rather put all the autogenerated stuff in a separate file and include! it or something, so at least it doesn't make searching a file a pain.

I think this is missing a bunch of #[cfg_attr(serde_derive, derive(Serialize)] annotations so that the updating instructions actually work isn't it?

8. Delete `expanded.rs`
9. Test your changes

This is very unfortunate, but it's better than keeping a stale dependency on `serde_derive`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I disagree with this FWIW.

#[derive(Clone, Deserialize, Debug)]
// Warning: Do not change this without regenerating serde_derive impls.
// See issue #203 and README.serde_derive for more information.
#[derive(Clone, Debug)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are all missing #[cfg_feature] bits right?

@emilio
Copy link
Collaborator

emilio commented Oct 15, 2018

Actually bindgen has the same issue (see rust-lang/rust-bindgen#1409)... Sigh. I'd rather go fix rust-lang/cargo#2589 instead fwiw.

@dtolnay
Copy link

dtolnay commented Oct 16, 2018

If the reason that this serde_derive dependency is a problem is that it enables proc-macro2's "proc-macro" feature and that ends up trying to dynamically link the compiler into cbindgen, then dtolnay/proc-macro2#139 (comment) may be a shorter path to fix that. We'd like for libproc_macro's build story to be no different from libcore or liballoc, meaning no more dynamic dependency on the compiler.

rust-lang/rust#49219 is much of the work to get there.

@eqrion
Copy link
Collaborator Author

eqrion commented Oct 16, 2018

I'm going to close this PR and put this on hold as it doesn't really sound like an attractive option. The work around libproc-macro sounds like a promising path.

@eqrion eqrion closed this Oct 16, 2018
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