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.

Sign up for GitHub

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

Jump to bottom

fix: verify msrv with rust-version fields (with cargo-msrv 0.16) #4941

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

fix: verify msrv with rust-version fields (with cargo-msrv 0.16) #4941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
50aa54c
chore: verify msrv with cargo-msrv 0.16
eitsupi Oct 9, 2024
32d6fb8
Merge branch 'main' into msrv-all
eitsupi Oct 9, 2024
bc8ca69
fix: fix rust-version
eitsupi Oct 9, 2024
1c853ef
fix: fix lutra's msrv and verify prqlc and lutra's msrv on CI
eitsupi Oct 9, 2024
abaf89a
fix: fix rust-version
eitsupi Oct 9, 2024
020f49a
ci: fix workdir
eitsupi Oct 9, 2024
23e249d
chore: comments
eitsupi Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions .github/workflows/tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,21 +513,22 @@ jobs:
runs-on: ubuntu-22.04
needs: rules
if: needs.rules.outputs.nightly == 'true'
strategy:
fail-fast: false
matrix:
working-directory:
# TODO: Verify workspace MSRV after https://github.com/foresterre/cargo-msrv/issues/590
# - ./
- prqlc/prqlc
- lutra/lutra
steps:
- name: 📂 Checkout code
uses: actions/checkout@v4
- uses: baptiste0928/cargo-install@v3
with:
crate: cargo-msrv
# TODO: remove this version pinning
# The latest 0.16 supports workspace inheritance, so the check will fail
version: "0.15"
# Note this currently uses a manually maintained key in
# `prqlc/prqlc/Cargo.toml`, because of
# https://github.com/foresterre/cargo-msrv/issues/590
- name: Verify minimum rust version — prqlc
# Ideally we'd check all crates, ref https://github.com/foresterre/cargo-msrv/issues/295
working-directory: prqlc/prqlc
- name: Verify minimum rust version — ${{ matrix.working-directory }}
working-directory: ${{ matrix.working-directory }}
run: cargo msrv verify

test-deps-min-versions:
Expand Down
7 changes: 4 additions & 3 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ authors = ["PRQL Developers"]
edition = "2021"
license = "Apache-2.0"
repository = "https://github.com/PRQL/prql"
# This isn't tested since `cargo-msrv` doesn't support workspaces; instead we
# test `metadata.msrv` in `prqlc`
rust-version = "1.70.0"
# This isn't tested since `cargo msrv verify` doesn't support workspaces
# https://github.com/foresterre/cargo-msrv/issues/590
# But we can find the MSRV by `cargo msrv find`
rust-version = "1.77.2"
Copy link
Member

Choose a reason for hiding this comment

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

If no crates have rust-version.workspace = true, does this have any effect?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?
lutra has rust-version.workspace = true

Copy link
Member

Choose a reason for hiding this comment

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

OK I see. Possible we push that down to lutra, and leave this empty.

Hopefully we can have an MSRV for most of the workspace set here, and then lutra can have its own higher one. But possibly that won't work yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, my understanding of MSRV was incorrect (foresterre/cargo-msrv#1023).

I will not work on it today so I will draft it and come back to it in a few days.

Copy link
Member

Choose a reason for hiding this comment

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

Ah great, thanks a lot for following up with that. I added a comment to the issue.

If that issue is resolved, I would vote to have an MSRV of something like 1.73.0 here, have most crates inherit it, and then Lutra has a different higher one.

version = "0.13.1"

[profile.release]
Expand Down
2 changes: 1 addition & 1 deletion prqlc/bindings/elixir/native/prql/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ authors = ["Kasun Vithanage <[email protected]>"]
description = "Elixir NIF bindings for prqlc"
name = "prql"
publish = false
rust-version = "1.73.0"

edition.workspace = true
license.workspace = true
repository.workspace = true
rust-version.workspace = true
version.workspace = true

[lib]
Expand Down
2 changes: 1 addition & 1 deletion prqlc/bindings/java/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
[package]
name = "prql-java"
publish = false
rust-version = "1.73.0"

edition.workspace = true
license.workspace = true
repository.workspace = true
rust-version.workspace = true
version.workspace = true

[lib]
Expand Down
2 changes: 1 addition & 1 deletion prqlc/bindings/js/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
description = "Javascript bindings for prqlc"
name = "prqlc-js"
publish = false
rust-version = "1.73.0"

edition.workspace = true
license.workspace = true
repository.workspace = true
rust-version.workspace = true
version.workspace = true

[lib]
Expand Down
2 changes: 1 addition & 1 deletion prqlc/bindings/prqlc-c/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
[package]
name = "prqlc-c"
publish = false
rust-version = "1.73.0"

edition.workspace = true
license.workspace = true
repository.workspace = true
rust-version.workspace = true
version.workspace = true

# This means we can build with `--features=default`, which can make builds more generic
Expand Down
2 changes: 1 addition & 1 deletion prqlc/bindings/prqlc-python/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ build = "build.rs"
description = "Python bindings for prqlc"
name = "prqlc-python"
publish = false
rust-version = "1.73.0"

edition.workspace = true
license.workspace = true
repository.workspace = true
rust-version.workspace = true
version.workspace = true

[lib]
Expand Down
2 changes: 1 addition & 1 deletion prqlc/prqlc-macros/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
[package]
description = "Macros for PRQL compilation at build time"
name = "prqlc-macros"
rust-version = "1.73.0"

edition.workspace = true
license.workspace = true
repository.workspace = true
rust-version.workspace = true
version.workspace = true

[lib]
Expand Down
2 changes: 1 addition & 1 deletion prqlc/prqlc-parser/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
[package]
description = "A parser for the PRQL query language."
name = "prqlc-parser"
rust-version = "1.73.0"

edition.workspace = true
license.workspace = true
repository.workspace = true
rust-version.workspace = true
version.workspace = true

[lib]
Expand Down
5 changes: 1 addition & 4 deletions prqlc/prqlc/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,9 @@ name = "prqlc"
edition.workspace = true
license.workspace = true
repository.workspace = true
rust-version.workspace = true
version.workspace = true

# Required for `cargo-msrv`, which doesn't yet support workspaces
metadata.msrv = "1.73.0"

rust-version = "1.73.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if this is the ideal situation as I believe the MSRV is modified by the cli feature.
(All downstream crates that don't use the cli feature would also be affected by this)

Copy link
Member Author

Choose a reason for hiding this comment

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

In my opinion, it is not a good idea to manage the compiler itself and the CLI in one crate (much less make it a default feature).

Copy link
Member

Choose a reason for hiding this comment

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

On the narrow question of whether the rust-version here is correct — it's fine if the version constraint could be a bit lower. We currently test that 1.73.0 covers all features. So I think we're good?

Copy link
Member Author

Choose a reason for hiding this comment

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

My point is that it is the cli feature that pushes the MSRV to 1.73, if excluding the feature, MSRV is 1.70.
However, there is no way to express that in the current Cargo.toml, so the rust-version here must be 1.73 and downstream crates cannot use Rust 1.70.

This is just the current situation, but my point is that it may be difficult to combine those that should keep MSRV as low as possible for downstream considerations and those that should not into a single crate and manage them with feature flags.
I'm fine with it as it is for now, but ideally the CLI should be separated into a separate crate like prqlc-cli.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree that's a downside with having them integrated. I do think there are benefits to having them in a single crate — in particular cargo install prqlc, but we have gone back & forth, there are tradeoffs...

Copy link
Member Author

Choose a reason for hiding this comment

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

I do think there are benefits to having them in a single crate — in particular cargo install prqlc, but we have gone back & forth, there are tradeoffs...

I do not see how there is any problem with cargo install prqlc changing to cargo install prqlc-cli.
Or rename other than CLI to something like prqlc-core.

In any case, this is a separate issue that needs to be taken up.

Copy link
Member Author

Choose a reason for hiding this comment

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

#4946

build = "build.rs"

[features]
Expand Down
2 changes: 1 addition & 1 deletion prqlc/prqlc/examples/compile-files/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
[package]
name = "compile-files"
publish = false
rust-version = "1.73.0"

edition.workspace = true
license.workspace = true
repository.workspace = true
rust-version.workspace = true
version.workspace = true

[[bin]]
Expand Down
Loading