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 crate to work with latest r2d2 and r2d2_postgres #10

Merged
merged 1 commit into from
Sep 19, 2015
Merged

fix crate to work with latest r2d2 and r2d2_postgres #10

merged 1 commit into from
Sep 19, 2015

Conversation

yberreby
Copy link
Contributor

No description provided.

@Ryman
Copy link
Member

Ryman commented Aug 20, 2015

First commit looks good but I'm unsure about the 2nd, running locally with features = ["openssl"] for postgres works fine for me? (with the type errors fixed)

I don't see any relation to the bug mentioned in the commit message. Have I missed something?

@yberreby
Copy link
Contributor Author

@Ryman rust-postgres has a cargo feature to conditionally compile ssl-related code, so that people who don't need Ssl can still use the crate without linking to the openssl system libraries.

Keeping the example as it was before updates broke the crate requires enabling this feature in the dependencies, thereby forcing users to build openssl even if they aren't going to use it. The obvious solution to let us have an example with SSL without forcing it upon library users would have been to use the dev-dependencies table in the .toml so that rust-postgres would be compiled with the openssl feature when testing, and without when using this crate from another one. However, as you can see in the issue I linked, this is buggy and users may still end up linking to openssl. Hence, the safest solution is not to use SSL in the example. The separate field for postgres isn't actually necessary, it can be listed as other dependencies - it's the remains of the tests I conducted locally. Fixing it now.

@Ryman
Copy link
Member

Ryman commented Aug 24, 2015

Thanks for the outline, I finally see the forest!

In future, we should reshape the api a bit so if ssl isn't even enabled then the api wouldn't ask for it, we'll be landing some changes to nickel itself soon which might affect this Middleware so I might handle that then.

For now, can you rebase to reword the commit messages to align with this convention and to squash the last commit too.

fix crate to compile with latest r2d2 and r2d2_postgres
disable SSL in the example to avoid unnecessarily linking to OpenSSL,
see [1]

[1]: #10 (comment)
@yberreby
Copy link
Contributor Author

Is there anything preventing this PR from being merged?

@Ryman
Copy link
Member

Ryman commented Sep 19, 2015

@filsmick Sorry, there's no notification when commits on a PR are changed so I didn't notice you'd updated this! Your changes LGTM.

Ryman added a commit that referenced this pull request Sep 19, 2015
fix(r2d2): updates for r2d2 and r2d2_postgres
@Ryman Ryman merged commit ca0bda3 into nickel-org:master Sep 19, 2015
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.

None yet

2 participants