Skip to content

Update serde (and others) - #159

Closed
jrozner wants to merge 1 commit into
mozilla:masterfrom
jrozner:update-serde-derive
Closed

jrozner wants to merge 1 commit into
mozilla:masterfrom
jrozner:update-serde-derive

Conversation

@jrozner

@jrozner jrozner commented Apr 4, 2018

Copy link
Copy Markdown

Saw the comment about #147 with respect to serde_derive but just tried building with stable (1.25) and everything seems to work correctly. Is there another reason this can't go in? We use a newer version of serde_derive in our application and pinning this version is causing issues.

@eqrion

eqrion commented Apr 5, 2018

Copy link
Copy Markdown
Collaborator

With #158 merged, I think we should no longer need the pinning. The reason I added the pinning originally was because some serdes would link to bad versions of syn, but with syn updated to be okay now this should be fine.

@jrozner

jrozner commented Apr 5, 2018 •

Copy link
Copy Markdown
Author

Cool. Updated the PR with the changes that were recently accepted before

Edit: It looks like this already came in through 54dcb1c. Should this be closed?

@eqrion

eqrion commented Apr 6, 2018

Copy link
Copy Markdown
Collaborator

And I was wrong, there is another issue here preventing us from removing the pinning and updating. I filed rust-lang/cargo#5304 to find a solution.

@kpcyrd

kpcyrd commented Aug 29, 2018

Copy link
Copy Markdown
Contributor

@eqrion Is this issue still present?

We currently ship serde-derive 1.0.70 in debian and the explicit dependency prevents us from uploading cbindgen.

It passes both cargo check and cargo test after removing the pin and running cargo update, unless you are aware of any problems we would patch the Cargo.toml and then upload the 0.6.3 release.

Thanks!

@eqrion

eqrion commented Aug 29, 2018 •

Copy link
Copy Markdown
Collaborator

@kpcyrd

I think this is still an issue. Does the binary run without crashing if you run it directly?

target/debug/cbindgen instead of cargo run

EDIT:
You may have to delete your Cargo.lock and build to see the issue. This is what happens with cargo install and where the issue was run into.

@kpcyrd

kpcyrd commented Aug 29, 2018

Copy link
Copy Markdown
Contributor

That's odd, running the binary directly doesn't work (running it through cargo run did work):

% target/debug/cbindgen -h
target/debug/cbindgen: error while loading shared libraries: libproc_macro-76e776d93c7dc9f0.so: cannot open shared object file: No such file or directory
%

It seems the cargo bug that was linked here was closed in favor of rust-lang/cargo#2589 which seems unrelated.

The relevant issue is probably rust-lang/rust#45601 with a work-in-progress solution in rust-lang/rust#49219.

I'm a bit confused that this only applies to cbindgen since a lot of programs depend on a recent serde_derive, including my own.

@eqrion

eqrion commented Aug 29, 2018

Copy link
Copy Markdown
Collaborator

I thought we had an issue filed with exactly what is going on here, but we don't exactly. I'll summarize it here for reference and file an issue later.

cbindgen depends on syn which depends on proc-macro-2 because syn is geared to be used in proc-macros not random binaries for parsing rust code. proc-macro-2 is the problematic library that causes this crash because it depends on internal compiler libs.

cbindgen doesn't need any of the functionality from syn that uses proc-macro-2 though, so we had a feature added to syn called proc-macro that we could use to disable this dependency. But disabling that feature is not enough to prevent proc-macro-2 from being linked.

Unfortunately proc-macro-2 is used by serde_derive as a build time proc-macro crate, and cargo does feature flag resolution once for build dependencies and binary dependencies. So our feature flag gets overridden because serde_derive needs that flag enabled. Even though serde_derive only happens at compile time, and we want the feature disabled at run time.

But not all serde_derive's have this dependency. Which is why we pin to a version which doesn't yet have it.

That's what rust-lang/cargo#2589 is about, and why it's still relevant. If that's fixed we could use the feature flag and update serde_derive.

I don't think we could easily drop the serde_derive requirement either. It's used for parsing cbindgen.toml which isn't required if all you care about is build.rs. But it is used for parsing Cargo.lock which is required for most uses of the crate.

This has been a major pain, and I'd love to find an answer to it.

@eqrion

eqrion commented Oct 1, 2018

Copy link
Copy Markdown
Collaborator

I'm going to close this as there's no good path forward. Once #203 is resolved we can update.

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