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

Use dynamic supported fuel-core version #1190

Merged
merged 22 commits into from
May 22, 2024

Conversation

Br1ght0ne
Copy link
Contributor

@Br1ght0ne Br1ght0ne commented Nov 7, 2023

Close #1189. Waiting on a new fuel-core release incorporating FuelLabs/fuel-core#1473.

Checklist

  • I have linked to any relevant issues.
  • I have updated the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary labels.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@Br1ght0ne Br1ght0ne self-assigned this Nov 7, 2023
@Salka1988 Salka1988 linked an issue Nov 8, 2023 that may be closed by this pull request
@Br1ght0ne Br1ght0ne added blocked fuel-core Requires more info or is blocked by fuel-core labels Nov 20, 2023
@Br1ght0ne Br1ght0ne marked this pull request as ready for review November 27, 2023 15:37
@Br1ght0ne Br1ght0ne marked this pull request as draft November 29, 2023 20:03
@Br1ght0ne Br1ght0ne marked this pull request as ready for review December 4, 2023 15:44
hal3e
hal3e previously requested changes Jan 16, 2024
packages/fuels-accounts/src/provider.rs Outdated Show resolved Hide resolved
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

I know this hasn't been your focus lately, but is there any updates on this?

@Br1ght0ne Br1ght0ne requested a review from hal3e February 12, 2024 15:15
iqdecay
iqdecay previously approved these changes Feb 13, 2024
Copy link
Contributor

@iqdecay iqdecay left a comment

Choose a reason for hiding this comment

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

Looks good! Just a question.

packages/fuels-accounts/fuel-core-version Outdated Show resolved Hide resolved
packages/fuels-accounts/build.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@segfault-magnet segfault-magnet left a comment

Choose a reason for hiding this comment

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

Some notes:

  • Using a build.rs is problematic because this will cause our users to always depend on fuel-core since we're packaging a build.rs dependent on fuel-core that will run when users build their software. One of the motivations for the fuel-core-lib is to lower compile times for live demos.
  • We need a CI check that when the fuel-core version in the workspace cargo toml is updated the corresponding version file matches the version in the toml.
  • If we're to generate version files we might as well generate a rust file that has a pub const version: ... = VERSION_HERE. Better yet if there is a const version structure so that we don't have to do any parsing in runtime.

Maybe it is enough to have that version.rs file with a unpublished binary that checks if the versions match. Something like the docs checker binary currently run in the CI.

Also keep in mind that sometimes our fuel-core versions are git checkouts, when there are circularly dependent breaking changes between the compiler, core and us. The CI should allow for this (unless we're publishing).

@Br1ght0ne Br1ght0ne force-pushed the oleksii/dynamic-supported-core-version branch from 0ccc474 to 125f3ac Compare March 25, 2024 16:16
@Br1ght0ne Br1ght0ne enabled auto-merge (squash) May 21, 2024 12:24
Cargo.toml Outdated Show resolved Hide resolved
MujkicA
MujkicA previously approved these changes May 21, 2024
scripts/fuel-core-version/Cargo.toml Outdated Show resolved Hide resolved
Salka1988
Salka1988 previously approved these changes May 21, 2024
Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

LGTM

@Br1ght0ne Br1ght0ne merged commit b706076 into master May 22, 2024
44 checks passed
@Br1ght0ne Br1ght0ne deleted the oleksii/dynamic-supported-core-version branch May 22, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuel-core Requires more info or is blocked by fuel-core package:provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use dynamic supported fuel-core version in version check Reminder to update fuel-core error [nice to have]
7 participants