/diesel

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

Our build is failing #1602

Closed
sgrif opened this Issue Mar 26, 2018 · 0 comments

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
1 participant
@sgrif
@sgrif
Member

sgrif commented Mar 26, 2018

I noticed every PR was red, so I decided to rebuild the latest commit on master. The clippy run is failing, due to cargo-metadata failing to compile. This is on a specific nightly version, so this must be due to a transitive dependency update.

@sgrif sgrif referenced this issue Mar 26, 2018

Merged

Bump num-traits to 0.2 #1595

sgrif added a commit that referenced this issue Mar 26, 2018

@sgrif
Don't enable the unstable feature when testing clippy 
Ok so this is a super subtle interaction. Our build randomly started
failing somewhere around a week ago. The `cargo-metadata` crate started
to fail to compile for no apparent reason. The failure came from
`#[derive(Deserialize)]` and the fact that fields on the struct were
private, which shouldn't ever happen.

This was specifically when we run clippy and compile-test, which is on a
pinned version of Rust. The version of `cargo-metadata` being resolved
hadn't changed, so that meant it must be serde. The version of serde
that gets resolved *did* change between the last passing build and the
first failing one. But I tried `cargo update` and then `bin/test`
locally and it passed, so I was really confused.

The difference between what we do in `bin/test` and the clippy run is
enable the `unstable` feature on `diesel`. This enables the `nightly`
feature on `diesel_derives`, which enables the `nightly` feature on
`proc-macros2`. This causes hygiene to come into play where it didn't
before, and as far as I'm aware serde does not work with hygiene turned
on.

The thing that changed is that serde updated its dependencies on
syn and proc-macro2. Which means they're now pointing at the same
version that we are. Which means that us enabling that feature is now
affecting that crate, since features are additive (and according to
cargo, every crate is supposed to work with any combination of features
both on itself and its dependencies even though that's not really
practical).

I'm not sure what to do about this long term. After 1.2 is released, I'm
planning on actively encouraging people to turn on the unstable feature
for development, since it so dramatically improves our error messages.
But doing that apparently makes us incompatible with serde now... So
¯\_(ツ)_/¯

Short term though, if we don't enable the unstable feature, we don't
have this problem. So let's not.

As an aside, I'm really shocked that `cargo-metadata` is the only
transitive dependency anywhere in our graph that hit this. Also why the
fuck does that crate have serde as a required dependency? Presumably the
only reason that it's the only crate that hit it is because everyone
else has it behind a feature *like they should*

Fixes #1602
5d482f3

@sgrif sgrif referenced this issue Mar 26, 2018

Merged

Don't enable the unstable feature when testing clippy #1603

@sgrif sgrif closed this in #1603 Mar 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment