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: support old Rust versions #1561

Closed
wants to merge 19 commits into from
Closed

Conversation

eitsupi
Copy link
Member

@eitsupi eitsupi commented Jan 18, 2023

While preparing to submit prqlr to CRAN, I noticed that prql-compiler 0.4.0 could not be built on older Rust versions.
The CRAN builders and the R-universe builders that emulate them do not yet support let-else on Linux because they use the distribution standard Rust.
Fortunately, there seemed to be quite a few things that needed to be changed, so I made the changes and am testing to see if the build succeeds by referencing this branch.

I do not want this branch merged because I think the developer experience will be degraded if let-else is not available.
I simply wanted to have the changes I made recorded in the form of a Pull Request.

(By the way, obviously my commit message is confusing, sorry.)

@aljazerzen
Copy link
Member

I was quite excited when I discovered this syntactic sugar, because it simplified quite a few code blocks. But it's not a big burden to merge this, especially knowing that once CRAN builders & Debian support let-else, we can revert this commit by running clippy with this lint.

So if this is the only obstacle which blocks releasing to CRAN, we can merge and lower MSRV.

Btw, do you know which rust version are they using?

@eitsupi
Copy link
Member Author

eitsupi commented Jan 18, 2023

Thanks for the reply!

Sorry, I am not sure of the exact Rust version. Probably 1.63 on Debian (testing)?
R-universe is 1.61 on Ubuntu 22.04.

However, when I submitted prqlr a month ago (PRQL/prqlc-r#26) and was rejected1, they were able to build prql-compiler v0.3.1 without any problems.
So I assume there are no other compatibility issues other than this.

Footnotes

  1. Because I violated the CRAN policy by writing the cargo cache to the builder's home directory. I've now fixed the Makefile to fix the problem.

@aljazerzen
Copy link
Member

If they are using 1.61 (May 2022) and let-else was released in 1.65 (November 2022), let's hope that in half a year, they will upgrade to 1.65.

@eitsupi
Copy link
Member Author

eitsupi commented Jan 18, 2023

For now, I pushed 996a783 to be able to install from GitHub. This should at least make the R-universe build work.......

@aljazerzen
Copy link
Member

Ah, we are also using workspace inheritance quite a lot. @max-sixty are we willing to sacrifice it for support of 1.61.0?

@eitsupi
Copy link
Member Author

eitsupi commented Jan 18, 2023

Thanks to changes on this branch, the builds for R-universe (Rust 1.61 on ubuntu:jammy Docker container) succeeded.

https://eitsupi.r-universe.dev/ui#package:prqlr
image

The build for Windows failed, but this is the same as for prql-compiler 0.3.1 and has nothing to do with this issue (still unknown cause......).

Except that I'm not happy that the source code uses my fork's URL instead of this repository (the old Cargo seems to have no choice but to use my fork's URL to refer to commits on this PR......), so I figure maybe I can submit this to CRAN.
https://github.com/eitsupi/prqlr/blob/0a59c952c948fdfd824bdad18a9f3ecdae2413e0/src/rust/Cargo.toml#L13

@max-sixty
Copy link
Member

I'm happy to extend our supported versions beyond n-1 going forward (defined here) — the new features are often nice, but not revolutionary...

Rolling back the workspace versioning would be quite a dreary task, so if we can avoid it, that would be very preferable. But if we really have to, 🫡 ! (and it's not difficult per se, would just be dreary work). Do we need to roll back to get this working or are we OK?

FWIW I've seen some vigorous debate in the rust about this; I can find the threads if anyone is interested (I remember @matklad giving some thoughtful commentary). Consensus seems to increasingly be that only supporting latest is reasonable, but by no means unanimous.

One issue to consider is that we are at the mercy of dependencies, and it's not considered a breaking change to increasing the min version. And we have no tests that we build at anything but the version in our rust-toolchain.toml. We could add a test for the version in our Cargo.toml — which is currently incorrect at 1.60. Would that be helpful?

@max-sixty
Copy link
Member

(and thank you very much @eitsupi for both uploading to CRAN and this PR)

@aljazerzen
Copy link
Member

cargo msrv tells me that:

  • our msrv is 1.65.0, because if let-else and workspace inheritance,
  • if we revert that, we could probably support 1.60.0

Our dependencies support is such:

├╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┤
│ 1.56.0 ┆ unsafe-libyaml, indexmap, sqlformat, pest_derive,     │
│        ┆ pest, once_cell, hashbrown, pest_generator, pest_meta │
├╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┤
│ 1.57.0 ┆ os_str_bytes                                          │
├╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┤
│ 1.58.0 ┆ serde_yaml                                            │
├╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┤
│ 1.60.0 ┆ prqlc, prql-compiler, clap_lex                        │
├╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┤
│ 1.64.0 ┆ clap, clap_derive                                     │
└────────┴───────────────────────────────────────────────────────┘

Fortunately, os_str_bytes, serde_yaml, clap_lex, clap & clap_derive are not dependencies of prql-compiler (but only the cli), so 1.56.0 is the minimum we could support.

As I said, I do think we could live a 6 months old Rust, if this means we can support CRAN.

@max-sixty
Copy link
Member

Added to CI: #1566

@max-sixty
Copy link
Member

So it sounds like there's some initial consensus on running at whatever CRAN allows. (And I'm guessing "upgrade rust" is not allowed in the build steps!)

Is there consensus on pausing upgrades and letting us catch up, rather than downgrading now?

@eitsupi are there any references we can link to for their policy?

@aljazerzen
Copy link
Member

Yeah, pause is ok too.

In this case this PR can remain until CRAN catches up to 1.65.0.

@eitsupi
Copy link
Member Author

eitsupi commented Jan 19, 2023

I really appreciate you considering this.

Rolling back the workspace versioning would be quite a dreary task, so if we can avoid it, that would be very preferable. But if we really have to, 🫡 ! (and it's not difficult per se, would just be dreary work). Do we need to roll back to get this working or are we OK?

If I understand correctly, this is not a problem when installing packages from crates.io, right?

If so, and if it is possible to install from crates.io, then installing from GitHub should not be a problem even if it does not work.

are there any references we can link to for their policy?

CRAN build machines can be found here. Linux machines are Fedora and Debian.
https://cran.r-project.org/web/checks/check_flavors.html

Fedora seems to be quickly getting Rust releases in, while Debian testing seems to be about 3 or 4 months behind.
https://src.fedoraproject.org/rpms/rust
https://tracker.debian.org/pkg/rustc

So perhaps a 4 month old version of Rust (3 releases ago?) would seem to be able to successfully build on the CRAN builder.

When I look into Ubuntu LTS, used on the R-universe, it appears to be lagging behind by up to 8 months, since it is only updated about once every 4 months and the release frequency is also about once every 4 months.
However, since R-universe is configured to replicate the CRAN builder as closely as possible, it is possible to negotiate to have the Rust installation method changed in the future to be closer to the Rust version on the CRAN builder.

@max-sixty
Copy link
Member

So perhaps a 4 month old version of Rust (3 releases ago?) would seem to be able to successfully build on the CRAN builder.

Great, let's do that. So we'll be on 1.65 until 1.69 comes out, and which point we'll upgrade to 1.66

If I understand correctly, this is not a problem when installing packages from crates.io, right?

I think that's right. If so, we can release to CRAN by either a) merging this to roll back the if else or b) release this branch.
Then we have a month of manual oversight to avoid re-introducing any new language features before our rule catches up with 1.65 and we're on automated checks again.

Does that make sense to everyone?

@eitsupi
Copy link
Member Author

eitsupi commented Jan 20, 2023

I submitted prqlr that installs 2513cfe (https://github.com/eitsupi/prqlr/blob/a76602e9809f9ffb6a04de87e359818e7680291c/src/rust/Cargo.toml#L14-L15) to CRAN this morning, and it appears to have been accepted!
https://cran.r-project.org/package=prqlr

The interval between submissions to CRAN should be no less than one month, so the next submission could coincide almost with the time when the Debian Rust version reaches 1.65.
So perhaps even without merging this branch, if the main stay with Rust 1.65, I can switch prqlr to installing from crates.io in the next CRAN release.

@max-sixty
Copy link
Member

Super @eitsupi ! Thanks for pushing that through.

Let me know if there's anything I can do. You're welcome to leave the PR open or close it and then reopen if needed.

@eitsupi
Copy link
Member Author

eitsupi commented Jan 21, 2023

Can I keep this open until prqlr can switch to installing prql-compiler from crates.io?
I will not be submitting to CRAN for a while, but may do some works on this branch for builds on R-universe to keep up with the prql-compiler version upgrades.

@eitsupi
Copy link
Member Author

eitsupi commented Feb 14, 2023

Contrary to expectations, the Rust version of Debian testing is still 1.63!
It is almost a month since the last submission and I would like to submit a new prqlr, but obviously I need to update this branch and continue installing from this branch......

@max-sixty
Copy link
Member

We'll freeze on 1.65 until it catches up! Even if it takes longer than we expected...

@eitsupi
Copy link
Member Author

eitsupi commented Feb 18, 2023

Thanks!
For now, I will submit prqlr 0.2.0 configured to install prql-compiler 0.5.1 from this branch.

@eitsupi
Copy link
Member Author

eitsupi commented Feb 18, 2023

prql 0.2.0 installs c9a1233 is now on CRAN.

@eitsupi eitsupi changed the title feat: support old Rust versions fix: support old Rust versions Feb 19, 2023
@eitsupi
Copy link
Member Author

eitsupi commented Mar 16, 2023

prqlr 0.3.0, based on cf4603d, is now on CRAN.

@eitsupi
Copy link
Member Author

eitsupi commented Apr 21, 2023

Hmmm......Due to the stuck Rust version of Debian, the time has come to update to 1.66 before it can be built on Debian default Rust....... (#2478)

It seems I still need to continue to maintain this branch to build prqlr.......

@max-sixty
Copy link
Member

max-sixty commented Apr 22, 2023

As long as Debian is slightly behind, happy to wait — we can push the requirement to latest+4!

If Debian is locked on 1.5x for a long time, then I guess we need to move on, but surely that's not the case?

@eitsupi
Copy link
Member Author

eitsupi commented Apr 23, 2023

Thanks!

If Debian is locked on 1.5x for a long time, then I guess we need to move on, but surely that's not the case?

Looking at https://tracker.debian.org/pkg/rustc, Debian testing's Rust version was bumped to 1.63 at 2022-12-13.
It has not been updated in the four months since then......

Debian experimental, which is upstream of testing (experimental -> unstable -> testing -> stable), is currently at 1.65 and work appears to be underway to update it to 1.66.
https://salsa.debian.org/rust-team/rust/-/merge_requests/24

I am not sure why no updates have come down from experimental to testing for such a long period.
Looking at the history, it seems that Rust 1.59 took 3 months to come down from experimental to testing. (2022-03-30 -> 2022-06-26)
experimental became 1.65 in March, so will testing become Rust 1.65 in June???

max-sixty added a commit to max-sixty/prql that referenced this pull request Apr 30, 2023
@eitsupi
Copy link
Member Author

eitsupi commented May 1, 2023

prqlr 0.4.0 which installs from prql-compiler 0.8.1 from a384174 was released on CRAN.

max-sixty added a commit to max-sixty/prql that referenced this pull request May 24, 2023
`rust-analyzer` is now raising mistaken errors for 1.65, as they only support the latest version rust-lang/rust-analyzer#12751 (comment)

We don't want to bump the required version because of PRQL#1561, but I think this approach:
- Lets us work on an updated version
- Tests `prql-compiler` & `prqlc` to ensure they don't fail to support 1.65
- Doesn't let us use any new features in `prql-compiler` or `prqlc` until we bump the required version, but that's completely fine (is there even _anything_ we'd use?)
@eitsupi
Copy link
Member Author

eitsupi commented Jun 26, 2023

It appears that Debian's Rust upgrade process is underway.
Hopefully soon Debian testing will get rustc 1.65 and I can close this PR.......
image
https://tracker.debian.org/pkg/rustc

@max-sixty
Copy link
Member

@eitsupi should we close this now?

@eitsupi
Copy link
Member Author

eitsupi commented Aug 1, 2023

I was thinking of waiting until prqlr 0.5.0 is released, but since I don't think it will probably be rejected because of the Rust version, I don't see a problem with closing this now.

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

3 participants