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

Fix Rocket integration #67

Closed
wants to merge 3 commits into from
Closed

Fix Rocket integration #67

wants to merge 3 commits into from

Conversation

monorkin
Copy link
Contributor

Rocket's traits have changed in version 0.3.0, this pull request adjusts the existing implementation to be compatible with the latest version of the traits.

Rocket's traits have changed in version 0.3.0, this commit adjusts
the existing implementation to be compatible with the latest
version of the traits.
@theduke
Copy link
Member

theduke commented Jul 29, 2017

@stankec great, thanks for the PR.

The build is failing on Windows on nightly due to ring not compiling.
I'll try to find out why, but I guess ring is just broken on Windows + nightly right now.

Should be good to merge.

@theduke
Copy link
Member

theduke commented Jul 29, 2017

@stankec also could you add information about the upgrade to CHANGELOG.md? ( the unreleased section)

If you don't get around to it, I'll bolt it on later.

@theduke theduke self-assigned this Jul 29, 2017
@monorkin
Copy link
Contributor Author

@theduke AppVeyor may be failing due to rust-lang/rust#42422

And I couldn't find the unreleased section in the CHANGELOG.md file, so as to not bork thing up, I'd prefer if you (or anybody that knows what they are doing) to do it.

Sorry for the inconvenience, It's just not clear to me what to do.

@theduke
Copy link
Member

theduke commented Jul 29, 2017

I just added it yesterday after your PR. ;)

@theduke
Copy link
Member

theduke commented Jul 29, 2017

@stankec the example in juniper/examples/rocket-server.rs also needs to be updated.

I need to investigate why this is not built on CI...

(unrelated to this PR, but we also need a better example for rocket in general #68 )

@monorkin
Copy link
Contributor Author

monorkin commented Jul 29, 2017

@theduke I did update the example. And I run the test suite, everything passed. I'm even running the example on my machine. (Started with cargo run --example rocket-server --features="rocket-handlers expose-test-schema")

Are you referring to the need for a better example?

@theduke
Copy link
Member

theduke commented Jul 29, 2017

Haha well you shouldn't look at code after 3 hours of sleep.

@stankec Sorry bout that.

@theduke
Copy link
Member

theduke commented Jul 29, 2017

FYI, I have just tried to build ring on Appveyor: https://ci.appveyor.com/project/theduke/ring/build/1.0.1

It's seems that Ring is definitely the problem
We need to investigate what the problem is, since all builds will fail on windows now. -.-

@mhallin
Copy link
Member

mhallin commented Jul 29, 2017

The tests are failing on Travis too, we're just ignoring failures on nightly: https://travis-ci.org/mhallin/juniper/jobs/258473732

It seems as if Rocket has removed or renamed their testing feature, so our integration tests need to be fixed.

As for the Ring build failure, it seems as if they're not compatible with the GCC toolchain: https://github.com/briansmith/ring and that's why it's failing. We could probably get by with disabling the Rocket integration for Windows+GCC builds.

@theduke
Copy link
Member

theduke commented Jul 29, 2017

@stankec as recorded in #70, we'd like to pull out rocket and iron integrations into juniper_rocket and juniper_iron crates.

Would handling the extraction for rocket be something you are up to?
If not, no worries, I'll do it tomorrow.

@monorkin
Copy link
Contributor Author

monorkin commented Jul 29, 2017 via email

@fbernier
Copy link

fbernier commented Aug 1, 2017

The tests are failing on nightly because this change is incomplete and the rocket integration tests fail. See the The 'testing' feature was removed. section here: https://github.com/SergioBenitez/Rocket/blob/master/CHANGELOG.md#breaking-changes

We need to replace this: https://github.com/mhallin/juniper/pull/67/files#diff-576d7531dece2e01a8a09d86ff09c869L155

Also, 👍 on creating new crates

Edit: I had missed mhallin's comment mentioning this. My bad.

@monorkin
Copy link
Contributor Author

monorkin commented Aug 2, 2017

Hey. This completely slipped my mind. My laptop literally caught fire on Monday morning. I'll get to it in the evening.

theduke and others added 2 commits August 3, 2017 08:37
Updated changelog with preliminary info about project restructure and custom derives.
This split still requires some work as the tests currently fail. This is
caused by the fact that some previously private modules can't be
accessed anymore, and that Rocket changet it's testing framework.
@theduke
Copy link
Member

theduke commented Aug 3, 2017

@stankec cheers!

I already started with moving the iron integration too.

Since this has a lot of overlap, I will include your commits in my branch and then do a combined merge request if that is fine with you.

This actually involves a lot of stuff... fixing the documentation, moving the tests, fixing the juniper Cargo.toml, etc.... -.-

@monorkin
Copy link
Contributor Author

monorkin commented Aug 3, 2017 via email

@theduke
Copy link
Member

theduke commented Aug 6, 2017

Merged as part of #73

@theduke theduke closed this Aug 6, 2017
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.

4 participants