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

Modify static buffer size via env var #1869

Merged
merged 14 commits into from
Aug 18, 2023
Merged

Modify static buffer size via env var #1869

merged 14 commits into from
Aug 18, 2023

Conversation

SkymanOne
Copy link
Contributor

@SkymanOne SkymanOne commented Aug 7, 2023

This is a trivial solution addressing #1471 by introducing a build script to ink_env crate which read the INK_STATIC_BUFFER_SIZE env var and appends the constant to a file. It has been tested to work.

Because the value is baked into the ink_env binaries, cargo clean must be run every time the buffer size is modified.

Checklist:

  • Introduce .env files in a contract project and propagate the env values to the core ink crates
  • Add integration test
  • Persist the size in the metadata

Follow-ups:

  • Allow cargo-contract to pass read contract's local .env file and append the env vars to the build command.
  • Persist, the static buffer size in the metadata. Should also be done via cargo-contract by reading the supplied env var.

@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2023

Codecov Report

Merging #1869 (672fddf) into master (c2af398) will decrease coverage by 0.02%.
Report is 8 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1869      +/-   ##
==========================================
- Coverage   52.97%   52.95%   -0.02%     
==========================================
  Files         212      212              
  Lines        6728     6728              
==========================================
- Hits         3564     3563       -1     
- Misses       3164     3165       +1     
Files Changed Coverage Δ
crates/env/src/engine/off_chain/impls.rs 40.35% <ø> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@SkymanOne SkymanOne marked this pull request as ready for review August 9, 2023 09:10
@SkymanOne SkymanOne requested review from cmichi, ascjones and a team as code owners August 9, 2023 09:10
@kvinwang
Copy link
Contributor

This is a trivial solution addressing #1471 by introducing a build script to ink_env crate which read the STATIC_BUFFER_SIZE env var and appends the constant to a file.

INK_STATIC_BUFFER_SIZE would be a better name.

crates/env/build.rs Outdated Show resolved Hide resolved
integration-tests/static-buffer/README.md Show resolved Hide resolved
@xgreenx
Copy link
Collaborator

xgreenx commented Aug 11, 2023

Do you want to implement .env during this PR or in follow-up PR?

@SkymanOne
Copy link
Contributor Author

Do you want to implement .env during this PR or in follow-up PR?

This is a cargo-contract related issue

@xgreenx
Copy link
Collaborator

xgreenx commented Aug 11, 2023

But it seems you try to use the same idea with the build script but use the CARGO_MANIFEST_DIR env variable that stores the .env file.

@SkymanOne
Copy link
Contributor Author

CARGO_MANIFEST_DIR will point to the ink_env folder because that's where the build is happening. Since the order of dependencies is contract -> ink -> ink_env, then ink_env build script will execute first. So, the idea is that we need to read .env file from cargo-contract and source it globally, so that the var is available during ink_env build

@cmichi
Copy link
Collaborator

cmichi commented Aug 16, 2023

My concern is that with the env variable we introduce another means of configuring the ink! environment, besides the trait. And that mean is outside of the usual Rust primitives.

It will be easy to overlook that the (hidden) .env file is missing during a build. This file will also need to be uploaded when using third party tools (such as block explorers contract verification or patron.works).

I'm in favor of merging #1872. It's also not an ideal solution, but in my opinion given the circumstances the best choice: setting this variable in Cargo.toml won't be overlooked or forgotten. It will also avoid the footgun with having to run cargo clean after each modification of the env variable.

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

On balance I think this is the lesser of two evils.

I don't think using an .env file is a good idea though. Instead we should take a leaf out of cargos book and allow it to be configured by (in order of precedence):

  • Command line arg e.g. cargo contract build --static-buffer-size 16384
  • Config in the contracts Cargo.toml using either a custom section or the [env] section
  • Just using the env var itself if set

cargo clean must be run every time the buffer size is modified.

This can be solved in cargo-contract by first checking the existing metadata for the existing buffer size, and running cargo clean if the new value is different.

@ascjones
Copy link
Collaborator

ascjones commented Aug 17, 2023

Note that a possible alternative implementation of this avoiding the build.rs would be to use const_env and do

#[from_env("INK_STATIC_BUFFER_SIZE")]
const BUFFER_SIZE: usize = 1 << 14;

@SkymanOne
Copy link
Contributor Author

On balance I think this is the lesser of two evils.

I don't think using an .env file is a good idea though. Instead we should take a leaf out of cargos book and allow it to be configured by (in order of precedence):

  • Command line arg e.g. cargo contract build --static-buffer-size 16384
  • Config in the contracts Cargo.toml using either a custom section or the [env] section
  • Just using the env var itself if set

cargo clean must be run every time the buffer size is modified.

This can be solved in cargo-contract by first checking the existing metadata for the existing buffer size, and running cargo clean if the new value is different.

Agreed. cargo-contract support is essential here.

Note that a possible alternative implementation of this avoiding the build.rs would be to use const_env and do

While it looks cleaner, it doesn't gives us much flexibility in terms of error reporting. However, I will test it out and see

@upendra-eth
Copy link

I am reaching out to seek an update on the status of the issue at hand. Could you kindly confirm if it has been successfully resolved? If so, I would be most grateful for your guidance on the specific changes required in my Ink contract. Additionally, I would like to know whether it is necessary to create a .env file in my contract folder. Your assistance and clarification on these matters would be greatly appreciated.

@SkymanOne SkymanOne mentioned this pull request Mar 4, 2024
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.

7 participants