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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stable Rust #969

Merged
merged 10 commits into from Jun 21, 2020
Merged

Stable Rust #969

merged 10 commits into from Jun 21, 2020

Conversation

konstin
Copy link
Member

@konstin konstin commented Jun 9, 2020

Fixes #5 馃殌
Fixes #210
Fixed #697

Based on #961, I wanted to find out what's still missing and it turns out it's not much!

I've moved the performance improvements through specialization behind a unstable feature gate. There still two tests failing, which afaik test the behaviour of the specialized functions I deactivated.

Once #961 is ready, you should be able to cherry pick 97f6aa6.

@davidhewitt
Copy link
Member

Also see #927 - though I think you go further here!

@konstin
Copy link
Member Author

konstin commented Jun 9, 2020

Sorry, totally missed #927. And nightly is definitely a better name for that feature.

@kngwyu
Copy link
Member

kngwyu commented Jun 10, 2020

@konstin
Thank you! In addition to trybuild tests, cargo clippy --all-features --all-targets looks failing

And nightly is definitely a better name for that feature.

Agreed.

@konstin konstin changed the title Poc stable rust (2 tests failing) Poc stable rust Jun 17, 2020
@kngwyu kngwyu marked this pull request as ready for review June 18, 2020 08:24
@kngwyu
Copy link
Member

kngwyu commented Jun 18, 2020

@konstin
I'm sorry but I used force push for rebasing on the master.

@kngwyu kngwyu changed the title Poc stable rust Stable rust Jun 18, 2020
@kngwyu kngwyu changed the title Stable rust Stable Rust Jun 18, 2020
@kngwyu
Copy link
Member

kngwyu commented Jun 18, 2020

Since this PR is almost ready, I think it's convenient to use this branch for migrating to stable.

Here's a TODO list:

  • Documentation
    • Find the minimum stable version and note it on README.
    • Document what is optimized by --features nightly
  • CI
    • Use 'nightly' instead of 'minimum-nightly'.
    • Move rustfmt and clippy tests from 'minimum-nihgtly' to a stable one.
    • Add --features nightly test only for Github Actions?
      • Hmm, but there are many tests on Actions and I don't want to make the things more complex 馃

@birkenfeld
Copy link
Member

Is slice_pattern really needed? If that's the only blocker, I'd propose to support at least 1.39 which is what RHEL8 is shipping, for example...

@kngwyu
Copy link
Member

kngwyu commented Jun 18, 2020

Is slice_pattern really needed?

Though I like slice_pattern and it simplifies our codes, I think we can avoid it.

If that's the only blocker, I'd propose to support at least 1.39 which is what RHEL8 is shipping, for example...

I'm not sure it's the only blocker, but happy to try to down the version 馃檪
Maybe we can support 1.36, which is the minimum required version of parking_lot.

@kngwyu
Copy link
Member

kngwyu commented Jun 18, 2020

I think we can support 1.39.0, but cannot support 1.38 because of const_vec_new.

@kngwyu
Copy link
Member

kngwyu commented Jun 18, 2020

Now the only TODO is documenting nightly flag, but where should the document be...? 馃

@davidhewitt
Copy link
Member

I suggest adding to advanced.md

@kngwyu
Copy link
Member

kngwyu commented Jun 19, 2020

@davidhewitt
Could you please take a glance at the documents?

@davidhewitt
Copy link
Member

馃憤 I'll do my best to find some time to read this all tonight

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks really brilliant! I've made some small suggestions to wording of documentation - pick the suggestions you like 馃槃

Cargo.toml Outdated
Comment on lines 41 to 42
# For CI
all-stable = ["default", "num-bigint", "num-complex"]
Copy link
Member

Choose a reason for hiding this comment

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

If this feature is just for convenience in CI scripts, maybe it's better to code it into the CI scripts instead of making this user-facing feature?

Copy link
Member

Choose a reason for hiding this comment

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

馃憤

@@ -4,6 +4,7 @@
[![Actions Status](https://github.com/PyO3/pyo3/workflows/Test/badge.svg)](https://github.com/PyO3/pyo3/actions)
[![codecov](https://codecov.io/gh/PyO3/pyo3/branch/master/graph/badge.svg)](https://codecov.io/gh/PyO3/pyo3)
[![crates.io](http://meritbadge.herokuapp.com/pyo3)](https://crates.io/crates/pyo3)
[![minimum rustc 1.39](https://img.shields.io/badge/rustc-1.39+-blue.svg)](https://rust-lang.github.io/rfcs/2495-min-rust-version.html)
Copy link
Member

Choose a reason for hiding this comment

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

鉂わ笍

guide/src/advanced.md Outdated Show resolved Hide resolved
guide/src/advanced.md Outdated Show resolved Hide resolved
guide/src/advanced.md Outdated Show resolved Hide resolved
guide/src/advanced.md Outdated Show resolved Hide resolved
guide/src/class.md Outdated Show resolved Hide resolved
guide/src/rust_cpython.md Outdated Show resolved Hide resolved
@kngwyu
Copy link
Member

kngwyu commented Jun 21, 2020

Thank you for the reviews!

@kngwyu kngwyu merged commit 7075827 into master Jun 21, 2020
@birkenfeld
Copy link
Member

Congrats, this is a huge step!

@konstin konstin deleted the poc-stable-rust branch June 21, 2020 09:44
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.

Get rid of specialization: Roadmap Get rid of specialization Compile with stable rust
4 participants