-
Notifications
You must be signed in to change notification settings - Fork 275
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
Relax cc dependency requirements. #103
Conversation
Yeah, we had some issues with updates of cc which broke our builds... But I think we should relax the requirements a little bit, given that the current pinning leads to issues too, for the second time now. Maybe we can relax it to a |
We definitely can't simply relax it out to 1.0.* again, as cc has a tendency to completely break a number of use-cases we care about because the maintainer doesn't care to support them, but we can bump it individually if people need it because of some other conflict. I'm not sure why cargo needs everything to have the same version of cc, seems like that could also be fixed in cargo. |
(If I didn't make a mistake in testing this), our build works on Rust 1.22 even with the most recent cc 1.0.32, so I suggest we pin it to I did some digging to understand what cargo does: Because |
Ah okay, there is indeed an issue for build dependencies: rust-lang/cargo#5237 |
There are versions <= 1.32.0 which will not work, so we can't just allow anything. It looks like to be compatible with libsodium we'd just need to bump the cc version. Why not do that?
… On Mar 29, 2019, at 09:20, Tim Ruffing ***@***.***> wrote:
Ah okay, there is indeed an issue for build dependencies: rust-lang/cargo#5237
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Oh I thought I had tested every version... but apparently I haven't. So the full truth is all versions on crates.io build for me except for 0.0.1 (haha, okay) and (1.0.27). So unless we ask them to yank 1.0.27, we need to decide if we want > 1.0.27 or < 1.0.27. Unfortunately we cannot exclude a single version, cargo just doesn't support that because all version requirements are combined with a logical AND.
Of course we can do that. I just feel that it does not tackle the core of the issue and next week we'll have the next open issue. I agree that we don't want to go back to 1.0.* (even though I'm not sure I fully agree with the statement that cc has a tendency of breaking things; it broke us once yes, but only once.) But pinning a single version seems overly restrictive to me. Unless we have different reasons such as we want to review every dependency we're using but I don't see that we're doing that currently. |
cc has broken us more than once, and upstream has an official policy is not caring, which is completely unacceptable for our stability requirements.
… On Mar 29, 2019, at 11:55, Tim Ruffing ***@***.***> wrote:
Oh I thought I had tested every version... but apparently I haven't.
So the full truth is all versions on crates.io build for me except for 0.0.1 (haha, okay) and (1.0.27). So unless we ask them to yank 1.0.27, we need to decide if we want > 1.0.27 or < 1.0.27. Unfortunately we cannot exclude a single version, cargo just doesn't support that because all version requirements are combined with a logical AND.
It looks like to be compatible with libsodium we'd just need to bump the cc version.
Of course we can do that. I just feel that it does not tackle the core of the issue and next week we'll have the next open issue. I agree that we don't want to go back to 1.0.* (even though I'm not sure I fully agree with the statement that cc has a tendency of breaking things; it broke us once yes, but only once.) But pinning a single version seems overly restrictive to me. Unless we have different reasons such as we want to review every dependency we're using but I don't see that we're doing that currently.
So a reasonable choice will be > 1.0.27, <= 1.0.32 then if you ask.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Okay but still does mean we need a specific version. |
Indeed. Mostly I'm hoping either (a) build time dependencies can conflict (this seems like a cargo limitation, they aren't themselves generating incompatible symbols so in theory this should be doable) or (b) the ecosystem matures more and issues like supporting very recent compilers become commonplace. Until then it's rather low effort to just bump the dep when a need arises.
… On Mar 29, 2019, at 09:44, Tim Ruffing ***@***.***> wrote:
Okay but still does mean we need a specific version.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@alekseysidorov Can you change the restriction to |
Sorry for delay I had deadline last week :) |
Cool, can you squash this? |
- set allowed versions range for the `cc` dependency
ready |
Pinned versions in stable libraries are anti pattern in my opinion, sometimes they lead to situations such this.