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

Fixes #1454 by setting the msrv #1463

Merged
merged 11 commits into from
Dec 28, 2023
Merged

Fixes #1454 by setting the msrv #1463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b003fef
sets MSRV for frb_codegen (wrongly to fail the tests) and implements …
patmuk Dec 21, 2023
8097db5
sets the correct MSRV
patmuk Dec 21, 2023
431a8a0
adds a (wrong) MSRV to frb_rust
patmuk Dec 21, 2023
5822266
changes the MSRV for frb_codegen to a wrong value, to test testing
patmuk Dec 21, 2023
f63eda9
changes testing for the MSRV by using an image with an explicit rust …
patmuk Dec 21, 2023
5584ca1
probably corrects the env var call
patmuk Dec 21, 2023
f1d760a
implemented env.FRB_MSRV_RUST_VERSION access differently
patmuk Dec 21, 2023
3e9918d
hardodes the MSRV
patmuk Dec 21, 2023
1b11265
sets the correct MSRV, but ci so that frb_rust passes, while frb_code…
patmuk Dec 21, 2023
72f8ad0
sets all correct values, so ci passes, as the final test
patmuk Dec 21, 2023
a880abe
small fixes
patmuk Dec 27, 2023
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
36 changes: 32 additions & 4 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,30 @@ jobs:
- run: ./frb_internal generate-website

# ----------------------------------- codegen -----------------------------------
# instead of running this explicit test for frb_codegen and frb_rust only, we run the test for
# all rust packages in the job "test_rust". While this would not be needed for these other packages,
# we avoid a ci dependency to cargo-msrv.
# However, the code is left commented here in case this decission changes.
# msrv:
# name: 'Test :: FRB Codegen :: MSRV'
# runs-on: ubuntu-latest
# steps:
# - uses: actions/checkout@v2

# - name: Install Rust
# uses: actions-rs/toolchain@v1
# with:
# toolchain: stable

# - name: Verify MSRV for frb_codegen
# run: |
# cargo install cargo-msrv
# cargo msrv --path frb_codegen verify

# - name: Verify MSRV for frb_rust
# run: |
# cargo install cargo-msrv
# cargo msrv --path frb_rust verify

generate_run_frb_codegen_command_generate:
name: 'Generate :: FRB Codegen :: Command Generate'
Expand Down Expand Up @@ -146,7 +170,7 @@ jobs:
flutter-version: ${{ env.FRB_MAIN_FLUTTER_VERSION }}
architecture: x64
- uses: taiki-e/install-action@cargo-llvm-cov

# execute
- run: ./frb_internal generate-run-frb-codegen-command-generate --set-exit-if-changed --package ${{ matrix.package }} --coverage

Expand Down Expand Up @@ -339,7 +363,11 @@ jobs:
version: nightly
- image: ubuntu-latest
version: nightly-2023-12-16
# TODO(@fzyzcjy, @gutenfries): add MSRV here, as well as other locked versions as needed
# tests the MSRV
# run on all rust packages, though only needed for frb_rust and frb_codegen
- image: ubuntu-latest
# update this, if a later MSRV is needed
version: 1.74.0

steps:
# setup
Expand Down Expand Up @@ -367,8 +395,8 @@ jobs:
- uses: actions/upload-artifact@v4
with:
name: ${{ github.job }}--${{ matrix.info.image }}--${{ matrix.info.version }}--coverage
path: target/coverage

path: target/coverage
test_dart_native:
name: 'Test :: Dart :: Native'
runs-on: ${{ matrix.image }}
Expand Down
1 change: 1 addition & 0 deletions frb_codegen/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
categories.workspace = true
description.workspace = true
edition.workspace = true
rust-version = "1.74.0"
Copy link
Owner

Choose a reason for hiding this comment

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

It may be great if we can lower the MSRV by fixing some trivial compile errors ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm - I personally don't mind, as I like to use the latest version.
But I suggest opening another issue for that. In #1454 you can get an impression of what kind of changes would have to be made (that was with rust 1.71., the compiler complained about pup (crate) mostly).
I assume that would require a redesign of the modularization.
IMHO: Not worth it, would not do that unless somebody complains.

I got to the MSRV of 1.74.0 by using cargo msrv, which performs a bisect search. So it won't compile to anything lower without changes to the code.
I would leave it as it, and only of somebody raises an issue needing a lower version work on specifically supporting from that version on.

Copy link
Owner

Choose a reason for hiding this comment

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

I suggest opening another issue for that

Sure! Then let's keep this PR simple and atomic.

keywords.workspace = true
license.workspace = true
name = "flutter_rust_bridge_codegen"
Expand Down
1 change: 1 addition & 0 deletions frb_rust/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[package]
name = "flutter_rust_bridge"
edition.workspace = true
rust-version = "1.70.0"
version.workspace = true
description.workspace = true
license.workspace = true
Expand Down
Loading