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

Fixes #1454 by setting the msrv #1463

Merged
merged 11 commits into from
Dec 28, 2023

Conversation

patmuk
Copy link
Contributor

@patmuk patmuk commented Dec 21, 2023

Changes

Checklist

  • An issue to be fixed by this PR is listed above.
  • New tests are added to ensure new features are working. End-to-end tests are usually in the ./frb_example/pure_dart example, more specifically, rust/src/api/whatever.rs and test/api/whatever_test.dart. => test is in ci
  • ./frb_internal precommit --mode slow (or fast) is run (it internal runs code generator, does auto formatting, etc).
  • If this PR adds/changes features, documentations (in the ./website folder) are updated.
  • CI is passing. (Operations are reproducible using corresponding ./frb_internal something commands shown in CI.)
    => all passes, except "Benchmark" jobs. They did not pass before this changes either.

Remark for PR creator

  • The ci test installs "crate msrv", which verifies that the version set is correct. It can be used to find the correct version number as well - but that should be done locally, not in ci - as the MSRV usually doesn't change ofter. The ci fails only when the MSRV is not working anymore, by installing that version and running cargo check.
  • This PR adds a MSRV to frb_codegen only, no other rust crates.
  • There are two ways to specify the MSRV in the cargo.toml:
    • [platform.rust-version] (which I choose) and
    • [platform.metadata.MSRV]

Copy link

welcome bot commented Dec 21, 2023

Hi! Thanks for opening this pull request! 😄

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0f344d2) 94.31% compared to head (a880abe) 99.20%.
Report is 304 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1463      +/-   ##
==========================================
+ Coverage   94.31%   99.20%   +4.88%     
==========================================
  Files         356      341      -15     
  Lines       13560    13141     -419     
==========================================
+ Hits        12789    13036     +247     
+ Misses        771      105     -666     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@patmuk
Copy link
Contributor Author

patmuk commented Dec 21, 2023

The ci build has some issues I would need some help with:

  1. The job Test :: Flutter :: Native:: iOS fails!
    The error is
[   +7 ms] executing: xcrun simctl spawn 90F52CAA-A5C8-4946-8432-DF0D4EF688AC log stream --style json --predicate eventType = logEvent AND processImagePath ENDSWITH "Runner" AND (senderImagePath ENDSWITH "/Flutter" OR senderImagePath ENDSWITH "/libswiftCore.dylib" OR processImageUUID == senderImageUUID) AND NOT(eventMessage CONTAINS ": could not find icon for representation -> com.apple.") AND NOT(eventMessage BEGINSWITH "assertion failed: ") AND NOT(eventMessage CONTAINS " libxpc.dylib ")

I don't understand how the changes lead to that - probably rust 1.74.0 is not the MSRV, but we need a higher number? Which are you using as the default in ci?

  1. The ci runs take much longer now! 53m! Though other (successful) runs take around 40 min - maybe that is a normal variation?

  2. I introduced a few warnings we should get rid of. Probably I am using a wrong settings for

    - uses: actions/checkout@v2
    - name: Install Rust
      uses: actions-rs/toolchain@v1

?

The warnings are:

[Test :: FRB Codegen :: MSRV](https://github.com/fzyzcjy/flutter_rust_bridge/actions/runs/7282632799/job/19845306520)
The following actions uses node12 which is deprecated and will be forced to run on node16: actions/checkout@v2, actions-rs/toolchain@v1. For more info: https://github.blog/changelog/2023-06-13-github-actions-all-actions-will-run-on-node16-instead-of-node12-by-default/
[Test :: FRB Codegen :: MSRV](https://github.com/fzyzcjy/flutter_rust_bridge/actions/runs/7282632799/job/19845306520#step:3:30)
The `set-output` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

(they are repeated a few times)

@patmuk
Copy link
Contributor Author

patmuk commented Dec 21, 2023

oh, 1. could be because of #1464. Can you maybe verify that it runs locally? I haven't set up docker, etc.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Dec 21, 2023

  1. could be because of Fix iOS CI which once in a while fails (flaky), while it works well locally #1464.

Usually, if one of the two ios test fails, it is just flaky, and ignore it. Also you can "bump" the CI by commiting some nonsense (e.g. a space change)

The ci runs take much longer now! 53m! Though other (successful) runs take around 40 min - maybe that is a normal variation?

No worries, run more as you progress, and we will see whether it is just a variation

The following actions uses node12

Again no worries until the last step. The existing CI has some warning as well due to 3rd party actions

@patmuk
Copy link
Contributor Author

patmuk commented Dec 21, 2023

Ok, super - will do so!

@fzyzcjy
Copy link
Owner

fzyzcjy commented Dec 27, 2023

Oh please ping me whenever I do not reply within a day! I did not see your reply above until now (when I open this page to refer to another issue).

Copy link
Owner

@fzyzcjy fzyzcjy left a comment

Choose a reason for hiding this comment

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

Good job!

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@@ -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.

@patmuk
Copy link
Contributor Author

patmuk commented Dec 27, 2023

Oh please ping me whenever I do not reply within a day! I did not see your reply above until now (when I open this page to refer to another issue).

Surely! No worries, I thought you might have a Christmas break (if you celebrate it).

@patmuk
Copy link
Contributor Author

patmuk commented Dec 27, 2023

Good job!

Thanks :) I am taking care of your remarks now.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Dec 28, 2023

Surely! No worries, I thought you might have a Christmas break (if you celebrate it).

No, as you can see from my github profile, I am on github 365 days per year ;)

@fzyzcjy fzyzcjy enabled auto-merge December 28, 2023 01:22
@fzyzcjy fzyzcjy merged commit 499ae26 into fzyzcjy:master Dec 28, 2023
76 checks passed
Copy link

welcome bot commented Dec 28, 2023

Hi! Congrats on merging your first pull request! 🎉

@fzyzcjy
Copy link
Owner

fzyzcjy commented Dec 28, 2023

@all-contributors please add @patmuk for code

Copy link
Contributor

@fzyzcjy

I've put up a pull request to add @patmuk! 🎉

@patmuk patmuk deleted the fixes-#1454-by-setting-the-MSRV branch December 30, 2023 18:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] cargo install 'flutter_rust_bridge_codegen@^2.0.0-dev.2' has compilation errors
2 participants