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

build: bump fbw dev env #8585

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

tracernz
Copy link
Member

Fixes #[issue_no]

Summary of Changes

Bump FBW dev env to the latest with SDK 0.24.

Screenshots (if necessary)

References

Additional context

Discord username (if different from GitHub):

Testing instructions

Check that the aircraft generally functions okay in SU14 (non-beta).

How to download the PR for QA

Every new commit to this PR will cause new A32NX and A380X artifacts to be created, built, and uploaded.

  1. Make sure you are signed in to GitHub
  2. Click on the Checks tab on the PR
  3. On the left side, click on the bottom PR tab
  4. Click on either flybywire-aircraft-a320-neo or flybywire-aircraft-a380-842 download link at the bottom of the page

@tracernz tracernz added this to the v0.12.0 milestone Mar 27, 2024
@github-actions github-actions bot added this to 🟡 Code Review: Ready for Review in Quality Assurance Mar 27, 2024
Quality Assurance automation moved this from 🟡 Code Review: Ready for Review to 🟣 QA Team Review: Ready to Test Mar 27, 2024
@2hwk 2hwk moved this from 🟣 QA Team Review: Ready to Test to ⌛ Awaiting Actions in Quality Assurance Mar 28, 2024
@2hwk 2hwk added Not Ready For Testing Not ready for testing as still being discussed or developed. SU15 and removed QA Ready to Test labels Mar 28, 2024
@Gurgel100
Copy link
Contributor

Please also adjust the Rust version to 1.74 in rust-toolchain.toml

@frankkopp
Copy link
Member

Please also adjust the Rust version to 1.74 in rust-toolchain.toml

I assume you tested this? A while ago when I updated the dev env this lead to compile issues.

@Saschl
Copy link
Member

Saschl commented Apr 3, 2024

Please also adjust the Rust version to 1.74 in rust-toolchain.toml

I assume you tested this? A while ago when I updated the dev env this lead to compile issues.

I did and everything worked fine 👍 In fact we could even go to 1.77 from what I could see. 1.74 is default on dev-env though. If we go to 1.77 without a new image, that would work but will cause download of rust dependencies on every build.

@2hwk
Copy link
Member

2hwk commented Apr 4, 2024

Note: This PR causes issues with builds running in SU14, do not merge until SU15 release.

@tracernz
Copy link
Member Author

tracernz commented Apr 4, 2024

Please also adjust the Rust version to 1.74 in rust-toolchain.toml

Bumped to the 1.74.1 patch release.

The dev-env only specifies 1.74 which is a bit loose for production IMO. Which exact release will it have in any given scenario?

-e- Might then wait for https://github.com/flybywiresim/dev-env/pull/16.

Looks like some fixes will also be needed to build with 1.74.

error: returning the result of a `let` binding from a block
   --> fbw-common/src/wasm/systems/systems/src/air_conditioning/acs_controller.rs:746:17
    |
736 | /                 let interpolation = (Self::UPPER_DUCT_TEMP_LIMIT_LOW_KELVIN
737 | |                     - Self::UPPER_DUCT_TEMP_LIMIT_HIGH_KELVIN)
738 | |                     / (Self::UPPER_DUCT_TEMP_TRIGGER_HIGH_CELSIUS
739 | |                         - Self::UPPER_DUCT_TEMP_TRIGGER_LOW_CELSIUS)
...   |
744 | |                         .get::<kelvin>())
745 | |                     + Self::UPPER_DUCT_TEMP_LIMIT_HIGH_KELVIN;
    | |______________________________________________________________- unnecessary `let` binding
746 |                   interpolation
    |                   ^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
    = note: `-D clippy::let-and-return` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::let_and_return)]`
help: return the expression directly
    |
736 ~                 
737 ~                 (Self::UPPER_DUCT_TEMP_LIMIT_LOW_KELVIN
738 +                     - Self::UPPER_DUCT_TEMP_LIMIT_HIGH_KELVIN)
739 +                     / (Self::UPPER_DUCT_TEMP_TRIGGER_HIGH_CELSIUS
740 +                         - Self::UPPER_DUCT_TEMP_TRIGGER_LOW_CELSIUS)
741 +                     * (zone_measured_temperature.get::<kelvin>()
742 +                         - ThermodynamicTemperature::new::<degree_celsius>(
743 +                             Self::UPPER_DUCT_TEMP_TRIGGER_LOW_CELSIUS,
744 +                         )
745 +                         .get::<kelvin>())
746 +                     + Self::UPPER_DUCT_TEMP_LIMIT_HIGH_KELVIN
    |

error: returning the result of a `let` binding from a block
   --> fbw-common/src/wasm/systems/systems/src/air_conditioning/acs_controller.rs:779:17
    |
769 | /                 let interpolation = (Self::LOWER_DUCT_TEMP_LIMIT_LOW_KELVIN
770 | |                     - Self::LOWER_DUCT_TEMP_LIMIT_HIGH_KELVIN)
771 | |                     / (Self::LOWER_DUCT_TEMP_TRIGGER_HIGH_CELSIUS
772 | |                         - Self::LOWER_DUCT_TEMP_TRIGGER_LOW_CELSIUS)
...   |
777 | |                         .get::<kelvin>())
778 | |                     + Self::LOWER_DUCT_TEMP_LIMIT_HIGH_KELVIN;
    | |______________________________________________________________- unnecessary `let` binding
779 |                   interpolation
    |                   ^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
help: return the expression directly
    |
769 ~                 
770 ~                 (Self::LOWER_DUCT_TEMP_LIMIT_LOW_KELVIN
771 +                     - Self::LOWER_DUCT_TEMP_LIMIT_HIGH_KELVIN)
772 +                     / (Self::LOWER_DUCT_TEMP_TRIGGER_HIGH_CELSIUS
773 +                         - Self::LOWER_DUCT_TEMP_TRIGGER_LOW_CELSIUS)
774 +                     * (zone_measured_temperature.get::<kelvin>()
775 +                         - ThermodynamicTemperature::new::<degree_celsius>(
776 +                             Self::LOWER_DUCT_TEMP_TRIGGER_LOW_CELSIUS,
777 +                         )
778 +                         .get::<kelvin>())
779 +                     + Self::LOWER_DUCT_TEMP_LIMIT_HIGH_KELVIN
    |
    
    error: unneeded `return` statement
    --> fbw-common/src/wasm/systems/systems/src/hydraulic/mod.rs:1757:9
     |
1757 |         return delta_vol / self.max_high_press_volume * fluid.bulk_mod();
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
     = note: `-D clippy::needless-return` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(clippy::needless_return)]`
help: remove `return`
     |
1757 -         return delta_vol / self.max_high_press_volume * fluid.bulk_mod();
1757 +         delta_vol / self.max_high_press_volume * fluid.bulk_mod()
     |

@tracernz
Copy link
Member Author

tracernz commented Apr 4, 2024

rust-lang/rust#115010 caused the fuel and payload sys to start failing. Seems they never intended this type of lifetime elision.

@Saschl
Copy link
Member

Saschl commented Apr 4, 2024

The dev-env only specifies 1.74 which is a bit loose for production IMO. Which exact release will it have in any given scenario?

What counts is the version in rust-toolchain.toml, it will override whatever is specified as default in the dev-env.

@tracernz
Copy link
Member Author

tracernz commented Apr 4, 2024

The dev-env only specifies 1.74 which is a bit loose for production IMO. Which exact release will it have in any given scenario?

What counts is the version in rust-toolchain.toml, it will override whatever is specified as default in the dev-env.

Indeed, but better to make them match so a download isn’t required. It takes a while.

@alepouna
Copy link
Contributor

SU15 has just been released, RTM?

@Saschl
Copy link
Member

Saschl commented May 23, 2024

There’s one sdk update we missed, might want to incorporate that so we only have the change once 👍

Does not work with our llvm installation and seems like the option is a custom addition by asobo
fbw-a32nx/src/wasm/fbw_a320/build.sh Outdated Show resolved Hide resolved
fbw-a32nx/src/wasm/fbw_a320/build.sh Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
fbw-a380x/src/wasm/fbw_a380/build.sh Outdated Show resolved Hide resolved
fbw-a380x/src/wasm/fbw_a380/build.sh Outdated Show resolved Hide resolved
fbw-common/src/wasm/terronnd/build.sh Outdated Show resolved Hide resolved
fbw-common/src/wasm/terronnd/build.sh Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge Not Ready For Testing Not ready for testing as still being discussed or developed. QA Tier 1 SU15
Projects
Quality Assurance
⌛ Awaiting Actions
Development

Successfully merging this pull request may close these issues.

None yet

6 participants