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

Maintain compatibility with Rust 1.25 #137

Merged
merged 6 commits into from
Dec 13, 2018

Conversation

azriel91
Copy link
Contributor

Issue #135

Not sure if you want to also include a CI config for Rust 1.18

@azriel91
Copy link
Contributor Author

azriel91 commented Dec 12, 2018

Implementation is incorrect for detecting rust features, shall find another way.


Pulled in the autocfg crate as a build dependency to do this, detects the compiler version instead of features though – can't find how to detect compiler features. Let me know if you prefer to go lower level and use rustc_version and do the version check in build.rs

@RalfJung
Copy link
Member

Not sure if you want to also include a CI config for Rust 1.18

I think that would be a good idea.

azriel91 added a commit to azriel91/failure that referenced this pull request Dec 12, 2018
The current latest (0.3.11)` is not compatible with Rust 1.18.0.
Pending <rust-lang/backtrace-rs#137>
@azriel91
Copy link
Contributor Author

Ah, 1.18.0 built on windows, but not linux. I'm off for tonight, but feel free to add to the branch if it's urgent.

@alexcrichton
Copy link
Member

This is unfortunately a bit too much code duplication for me to be comfortable maintaining purely for the reason of supporting older compilers. Is there a compelling reason to support 1.18.0?

@azriel91
Copy link
Contributor Author

Hm, besides "make the failure PR pass", I think that's better answered by @BurntSushi

@BurntSushi
Copy link
Member

I've honestly lost a lot of my own motivation for tracking older Rust releases. It is too much work, and in particular, is completely intractable without community consensus on what to do. I think we should at the very minimum put a specific Rust version in our CI config so that folks can at least know which version is supported though.

@RalfJung
Copy link
Member

error_len is stable since Rust 1.20. Assuming that this removes most of the need for code duplication, would it be okay for failure to bump its minimal version to 1.20?

@alexcrichton
Copy link
Member

Heh to add to what @BurntSushi says I've also lost motivation to even list the minimum Rust version in CI, all it ends up doing for me is making it annoying to land PRs that bump it (as you always forget about it) and also confusing contributors who aren't typically aware that it can be bumped.

It looks like the main duplication here is happening at the rust 1.25.0 boundary, so it seems fine to me to land whatever's necessary to support 1.25.0+ as it likely doesn't involve much duplication

@RalfJung
Copy link
Member

Oh dang I missed the repr(align) thing and that it requires Rust 1.25.

@azriel91 azriel91 force-pushed the compatibility/rust-1-18 branch from 9c54a2f to e50b0dc Compare December 12, 2018 21:07
@azriel91
Copy link
Contributor Author

Okay, so with 1.25.0 as the minimum, things are much easier!
Actually many times when Rust releases a new stable version, I think "wow, this should be the minimum stable" 😅.

use core::mem;
use core::slice;
#[cfg(not(feature = "std"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm what's going on here? Was this something we moved from std to core in the last few versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap, based on issue rust-lang/rust#49319 and the corresponding PR: rust-lang/rust#49698 which happened on 2018-04-12, which is just after Rust 1.26 was released.

@alexcrichton
Copy link
Member

Looks great to me! Just one thing I'm curious about, and it also looks like CI may be failing for 1.25.0? (I don't think std has c_void by that point)

@azriel91
Copy link
Contributor Author

azriel91 commented Dec 13, 2018

Yeap, took longer than I'd've liked to figure out that no_std support isn't able to be supported until Rust 1.30. I've updated the CI config to test the features with std, though it duplicates nearly all of the main script. Yaml doesn't have a standard merge syntax (probably for good reason).

@azriel91 azriel91 changed the title Include Rust 1.18 compatible implementations. Include Rust 1.25 compatible implementations. Dec 13, 2018
@azriel91 azriel91 changed the title Include Rust 1.25 compatible implementations. Maintain compatibility with Rust 1.25 Dec 13, 2018
@azriel91
Copy link
Contributor Author

azriel91 commented Dec 13, 2018

So, the "gimli-symbolize" feature transitively uses the parity-wasm crate (backtrace -> addr2line 0.7 -> object 0.9 -> parity-wasm 0.31.3).

parity-wasm makes use of auto dereferencing match patterns introduced in Rust 1.26.

Road from here, things that I'd consider unfeasible (maintenance cost, sanity):

  • Chase down every crate and make it backward compatible
  • Track down Rust compatibility for every dependency and introduce Rust version checks for each.

I've removed it from the 1.25 CI tests.

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Dec 13, 2018
The latest backtrace release isn't compatible with 1.26.2. A patch for
this is underway in:

rust-lang/backtrace-rs#137

Though for now, let's just restrict the version since we should be
bumping our minimum rustc requirement soon-ish anyway.
No need to test exhaustiveness, we can rely on that through bug reports!
@alexcrichton alexcrichton merged commit 17b4fd8 into rust-lang:master Dec 13, 2018
@alexcrichton
Copy link
Member

Thanks so much!

rh-atomic-bot pushed a commit to coreos/rpm-ostree that referenced this pull request Dec 13, 2018
The latest backtrace release isn't compatible with 1.26.2. A patch for
this is underway in:

rust-lang/backtrace-rs#137

Though for now, let's just restrict the version since we should be
bumping our minimum rustc requirement soon-ish anyway.

Closes: #1713
Approved by: cgwalters
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