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

chore(dependencies): update openssl to 0.7 and cookie to 0.2 #687

Merged
merged 2 commits into from
Nov 22, 2015

Conversation

seanmonstar
Copy link
Member

Closes #686

@Ryman
Copy link
Contributor

Ryman commented Nov 20, 2015

Does this not warrant a version bump since it will break downstream crates which depend on cookies = "0.1" which interop with hyper?

@seanmonstar
Copy link
Member Author

@Ryman how so?

@Ryman
Copy link
Contributor

Ryman commented Nov 20, 2015

If lib foo depends on cookie 0.1 to build a CookieJar and passes it to hyper::Cookie::from_cookie_jar then it'll be a version mismatch I think, as cargo will not use the same version between foo and hyper as they are semver incompatible?

i.e. error: expected CookieJar, found CookieJar (with different crate versions)

@Ryman
Copy link
Contributor

Ryman commented Nov 20, 2015

I've confirmed that when I pull this branch and use it as a local override, then nickel-cookies fails to build:

src/cookies.rs:55:52: 55:55 error: mismatched types:
 expected `&cookie::jar::CookieJar<'_>`,
    found `&cookie::jar::CookieJar<'_>`
(expected struct `cookie::jar::CookieJar`,
    found a different struct `cookie::jar::CookieJar`) [E0308]
src/cookies.rs:55                 header::SetCookie::from_cookie_jar(jar)
                                                                     ^~~
src/cookies.rs:55:52: 55:55 help: run `rustc --explain E0308` to see a detailed explanation
src/cookies.rs:55:52: 55:55 note: Perhaps two different versions of crate `cookie` are being used?
src/cookies.rs:55                 header::SetCookie::from_cookie_jar(jar)

I don't mind fixing it in this case, but this kind of thing is a breaking change which in some cases may be non-trivial to fix :(

@seanmonstar
Copy link
Member Author

Ah, I see. That is irritating. I think the cookie type should probably be exported somewhere in hyper, so that it can be used without also having to depend explicitly on cookie. Such as in header::shared::Cookie.

@Ryman
Copy link
Contributor

Ryman commented Nov 20, 2015

Yeah, it can be a pain. The re-exports should help in future, I think, but (at least at the time) there were suggestions that it might not be a good practice to follow in general though on an issue I logged about this: rust-lang/cargo#1636 (comment)

@seanmonstar
Copy link
Member Author

Hm, I had read the opposite just recently, that a crate should probably
reexport types that it depends on. I can't recall where I read that,
though.

Hyper has reexported Url since forever, and I haven't heard of issues about
that.

On Fri, Nov 20, 2015, 12:05 PM Ryman [email protected] wrote:

Yeah, it can be a pain. The re-exports should help in future, I think, but
(at least at the time) there were suggestions that it might not be a good
practice to follow in general though on an issue I logged about this: rust-lang/cargo#1636
(comment)
rust-lang/cargo#1636 (comment)


Reply to this email directly or view it on GitHub
#687 (comment).

@Ryman
Copy link
Contributor

Ryman commented Nov 20, 2015

I think it's only an issue when there's a diamond of dependencies, where two crates re-export something from another, and a downstream crate has to use both of these together. I think that's pretty rare though and warrants a more stern look at deciding what upstream versions to support in that case.

So my opinion is that re-exporting cookie (or whatever else) is worth doing. 👍

seanmonstar added a commit that referenced this pull request Nov 22, 2015
chore(dependencies): update openssl to 0.7 and cookie to 0.2
@seanmonstar seanmonstar merged commit 072d4fe into master Nov 22, 2015
@seanmonstar seanmonstar deleted the ssl-and-cookie-up branch November 22, 2015 02:32
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