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

migrate encoding v1 to sway-features #6586

Merged
merged 28 commits into from
Oct 28, 2024
Merged

migrate encoding v1 to sway-features #6586

merged 28 commits into from
Oct 28, 2024

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Sep 25, 2024

Description

This PR starts the implementation of https://github.com/FuelLabs/sway-rfcs/blob/master/rfcs/0013-changes-lifecycle.md.

CLI flags

Now, all CLIs, including our tests, support two new arguments

...
      --experimental <EXPERIMENTAL>
          Comma separated list of all experimental features that will be enabled [possible values: new_encoding, storage_domains]
      --no-experimental <NO_EXPERIMENTAL>
          Comma separated list of all experimental features that will be disabled [possible values: new_encoding, storage_domains]
...

Experimental features can also be enabled inside Forc.toml for workspaces and individual projects.

experimental = { encoding-v1 = true }

And can be enabled using environment variables:

FORC_NO_EXPERIMENTAL=feature_a,feature_b FORC_EXPERIMENTAL=feature_c forc build ...

These configurations are applied in a very specific order to allow them to be overwritten. The order from the weakest to the strongest is:
1 - Workspace TOML
2 - Project TOML
3 - CLI
4 - Environment variables.

The rationale is:
1 - We want an easy way to enable and/or disable a feature for the whole workspace;
2 - But we want to allow projects to overwrite these features, when needed.

These two are the suggested approach to configure experimental features, but we also want to allow someone to easily turn on or off features to test something in particular. For that one can use the CLI flags; and in the specific case one does not have access to the CLI, for example, when one of the SDK is compiling the code, one can still completely overwrite the TOML files using environment variables.

We are also applying the "no experimental" first. This is to allow the use case of exactly controlling which features will be enabled and disabled. In this case one can use the "meta-feature" all to disable everything and enable only the desired features. For example:

FORC_NO_EXPERIMENTAL=all FORC_EXPERIMENTAL=new_encoding your_command....

Test for "-h"

This PR also introduces snapshot tests for "-h" for all forc commands at https://github.com/FuelLabs/sway/pull/6586/files#diff-20a5e677d2ae9266a2247739b6a473d2ad0c627955187d668822e7d194f8e940

Multiple test.toml

This PR also enables the use of multiple test.toml. Now each test.toml will be a different test, allowing the same code to be compiled and asserted differently.

For example, main_args_empty has two files: the normal test.toml and a new test.encoding_v1.toml. One has new_encoding enabled and the other disabled.

When a test.toml file has the experimental field, we do not use fields with _new_encoding suffix anymore, making these files simpler:

test.toml:

category = "run"

# (1336, 1)
script_data = "0000000000000538 0000000000000001"
expected_result = { action = "return", value = 1337 }

validate_abi = true

experimental = { encoding_v1 = false }

test.encoding_v1.toml

script_data = "0000000000000538 0000000000000001"
expected_result = { action = "return_data", value = "0000000000000539" }

experimental = { new_encoding = true }

Test tomls with a suffix use test.toml as a base. So we do not need to repeat every field in them.

Now when we run a test, we will see two tests instead of just one:

...
Testing should_pass/language/main_args/main_args_empty (test.encoding_v1.toml) ... ok
Testing should_pass/language/main_args/main_args_empty (test.toml) ... ok
...

Conditional compilation

It is also possible to use conditional compilation using these experimental features. Examples can be found inside sway-lib-std.

#[cfg(experimental_new_encoding = true)]
pub fn assert_eq<T>(v1: T, v2: T) {
...
}

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • 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.

@xunilrj xunilrj force-pushed the xunilrj/sway-features branch 2 times, most recently from 2e7e798 to cc5c455 Compare September 25, 2024 19:00
Copy link

codspeed-hq bot commented Sep 25, 2024

CodSpeed Performance Report

Merging #6586 will improve performances by 10.96%

Comparing xunilrj/sway-features (a40edf5) with master (34b70f2)

Summary

⚡ 1 improvements
✅ 21 untouched benchmarks

Benchmarks breakdown

Benchmark master xunilrj/sway-features Change
document_symbol 5.2 ms 4.7 ms +10.96%

@xunilrj xunilrj force-pushed the xunilrj/sway-features branch 2 times, most recently from dbd8d28 to 30f2f05 Compare September 30, 2024 19:06
@xunilrj xunilrj marked this pull request as ready for review October 1, 2024 15:44
Copy link
Member

@ironcev ironcev left a comment

Choose a reason for hiding this comment

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

Great too see rich infrastructure for feature flags getting implemented! Just one quick comment after glancing over the PR description. Overall, the described infrastructure and intended usage looks great! ❤️

@xunilrj xunilrj force-pushed the xunilrj/sway-features branch from 65cec66 to 00c666a Compare October 21, 2024 10:52
@xunilrj xunilrj requested review from a team as code owners October 21, 2024 10:52
@xunilrj xunilrj force-pushed the xunilrj/sway-features branch from 5292f16 to 567c7d7 Compare October 22, 2024 19:22
Copy link
Contributor

@jjcnn jjcnn left a comment

Choose a reason for hiding this comment

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

These configurations are applied in a very specific order to allow them to be overwritten. The order from the weakest to the strongest is:
1 - Workspace TOML
2 - Project TOML
3 - CLI
4 - Environment variables.

Are you sure you want environment variables overwriting CLI and not the other way around? It feels like that could lead to some confusion for the user when he's passing a CLI argument that gets ignored because of an environment variable he didn't realize had been set.

Another thing is the use of both experimental and no-experimental. What happens if you give contradictory settings? And what happens if you fail to give a setting for a feature - presumably we fall back on a default setting, but wouldn't it then be better to only have the experimental option, and use the default setting if no user-defined setting is given?

@xunilrj
Copy link
Contributor Author

xunilrj commented Oct 24, 2024

Are you sure you want environment variables overwriting CLI and not the other way around?

If CLI dominates, developers without access to forc cannot overwrite these features. Example: you compile through SDK or an other tool. In this case, you don't necessarily have access to the CLI and may not be able to control which features you want on or off.

Another use case is to overwrite them in a CI environment easily.

What we may do, is print a warning, that some features are being overwritten by environment variables.

Another thing is the use of both experimental and no-experimental. What happens if you give contradictory settings?

"Contradictory settings" is actually a feature that we want eventually. If you want to test a specific set of settings you do FORC_NO_EXPERIMENTAL=all FORC_EXPERIMENTAL=feature_a. So you can precisely control which features are on and off.

And what happens if you fail to give a setting for a feature - presumably we fall back on a default setting, but wouldn't it then be better to only have the experimental option, and use the default setting if no user-defined setting is given?

Yes, we fallback to default. But without "NO_EXPERIMENTAL", if an experimental feature is enabled by default, there is no way to turn it off.

@xunilrj xunilrj force-pushed the xunilrj/sway-features branch from 95e8292 to cc89e4c Compare October 25, 2024 14:28
@xunilrj
Copy link
Contributor Author

xunilrj commented Oct 27, 2024

forc doc help is adapting itself to the terminal size, which means it gives different results locally and inside Github CI. I will try to fix it later.

@xunilrj xunilrj requested a review from ironcev October 27, 2024 08:09
Copy link
Member

@ironcev ironcev left a comment

Choose a reason for hiding this comment

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

Looking forward to get this PR merged and start incorporating experimental flags in two features I work on right now 😄

Just one additional remark. The support for E2E tests is great, but we will down the road also need to decide what to do with, e.g. SDK tests or Sway language tests. The former run precompiled Sway binaries which will for the time being be compiled without experimental features. The latter just run forc test so again, without experimental features. I expect once we start getting first PRs with experimental features, we can discuss the approach to bringing these tests to run with experimental features as well.

@IGI-111 IGI-111 merged commit ddb505d into master Oct 28, 2024
40 checks passed
@IGI-111 IGI-111 deleted the xunilrj/sway-features branch October 28, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants