improved validation for scarb version and scarb.toml config #2900

PoulavBhowmick03 · 2025-01-30T19:08:49Z

Closes #2710

Introduced changes

Checklist

crates/forge-runner/src/backtrace.rs

crates/forge/src/run_tests/workspace.rs

… into improve_scarb_validation

ksew1

Please add tests similar to found here:

https://github.com/foundry-rs/starknet-foundry/blob/master/crates/forge/tests/e2e/coverage.rs

Add delete this:

starknet-foundry/crates/forge-runner/src/backtrace.rs

Line 86 in 8ce451e

    
           "perhaps the contract was compiled without the following entry in Scarb.toml under [profile.dev.cairo]:

And this:

starknet-foundry/crates/forge-runner/src/backtrace.rs

Line 95 in 8ce451e

    
           "perhaps the contract was compiled without the following entry in Scarb.toml under [profile.dev.cairo]:

ksew1 · 2025-02-03T12:43:41Z

crates/forge-runner/src/backtrace.rs

+    Ok(())
+}
+
+pub fn is_backtrace_enabled() -> bool {


Use this function also here:

starknet-foundry/crates/forge-runner/src/backtrace.rs

Line 42 in f25f823

let is_backtrace_enabled = env::var(BACKTRACE_ENV).is_ok_and(|value| value == "1");

ksew1 · 2025-02-03T12:51:00Z

crates/forge-runner/src/backtrace.rs

+    env::var(BACKTRACE_ENV).is_ok_and(|value| value == "1")
+}
+
+fn contains_entry_with_value(table: &Table, key: &str, value: &str) -> bool {


fn contains_entry_with_value(table: &Table, key: &str, value: bool) -> bool { table .get(key) .and_then(toml_edit::Item::as_bool) .is_some_and(|entry| entry == value) }

Should we be taking value as a bool? Shouldn't it be a &str? @ksew1

It should be bool for 100%

… into improve_scarb_validation

ddoktorski · 2025-02-04T09:58:27Z

@PoulavBhowmick03

Please fix:

Failing test
Lint (by running cargo lint and addressing warnings - docs reference)

ddoktorski · 2025-02-06T09:20:37Z

@PoulavBhowmick03 any progress on this?

PoulavBhowmick03 · 2025-02-07T17:11:26Z

@PoulavBhowmick03 any progress on this?

apologies, was a little acquired, i will fix and modify the changes by tonight or tomorrow at max

… into improve_scarb_validation

PoulavBhowmick03 · 2025-02-10T03:58:01Z

@ddoktorski PTAL

ksew1 · 2025-02-10T10:40:57Z

crates/forge-runner/src/backtrace.rs

 const BACKTRACE_ENV: &str = "SNFORGE_BACKTRACE";
+const MINIMAL_SCARB_VERSION: Version = Version::new(2, 8, 0);

+const BACKTRACE_REQUIRED_ENTRIES: [(&str, &str); 2] = [


Suggested change

const BACKTRACE_REQUIRED_ENTRIES: [(&str, &str); 2] = [

const BACKTRACE_REQUIRED_ENTRIES: [(&str,bool); 2] = [

ksew1 · 2025-02-10T10:43:30Z

crates/forge-runner/src/backtrace.rs


-    let is_backtrace_enabled = env::var(BACKTRACE_ENV).is_ok_and(|value| value == "1");
+    let is_backtrace_enabled = is_backtrace_enabled();
    if !is_backtrace_enabled {


Suggested change

if !is_backtrace_enabled {

if !is_backtrace_enabled() {

ksew1 · 2025-02-10T10:48:10Z

crates/forge-runner/src/backtrace.rs

+
+    let manifest = fs::read_to_string(&scarb_metadata.runtime_manifest)?.parse::<DocumentMut>()?;
+
+    let profile_cairo = manifest


Maybe like this:

let has_needed_entries = manifest .get("profile") .and_then(|profile| profile.get(&scarb_metadata.current_profile)) .and_then(|profile| profile.get("cairo")) .and_then(|cairo| cairo.as_table()) .is_some_and(|profile_cairo| { BACKTRACE_REQUIRED_ENTRIES .iter() .all(|(key, value)| contains_entry_with_value(profile_cairo, key, *value)) }); ensure!( has_needed_entries, formatdoc! { "Scarb.toml must have the following Cairo compiler configuration to run backtrace: [profile.{profile}.cairo] unstable-add-statements-functions-debug-info = true unstable-add-statements-code-locations-debug-info = true ... other entries ... ", profile = scarb_metadata.current_profile }, );

ksew1 · 2025-02-10T10:51:01Z

crates/forge-runner/src/backtrace.rs

-                "perhaps the contract was compiled without the following entry in Scarb.toml under [profile.dev.cairo]:
-                unstable-add-statements-code-locations-debug-info = true
-
+            VersionedCoverageAnnotations::try_from_debug_info(sierra_debug_info).context(


Suggested change

VersionedCoverageAnnotations::try_from_debug_info(sierra_debug_info).context(

VersionedCoverageAnnotations::try_from_debug_info(sierra_debug_info)?;

ksew1 · 2025-02-10T10:51:13Z

crates/forge-runner/src/backtrace.rs

-                "perhaps the contract was compiled without the following entry in Scarb.toml under [profile.dev.cairo]:
-                unstable-add-statements-functions-debug-info = true
-
+            VersionedProfilerAnnotations::try_from_debug_info(sierra_debug_info).context(


Suggested change

VersionedProfilerAnnotations::try_from_debug_info(sierra_debug_info).context(

VersionedProfilerAnnotations::try_from_debug_info(sierra_debug_info)?;

crates/forge/tests/e2e/backtrace.rs

.tool-versions

PoulavBhowmick03 · 2025-02-10T12:09:19Z

@ksew1 PTAL

ksew1

Please run cargo fmt before pushing

ksew1 · 2025-02-10T13:38:05Z

crates/forge/tests/e2e/backtrace.rs

Pleas add similar test:

starknet-foundry/crates/forge/tests/e2e/coverage.rs

Line 40 in ce5cb88

fn test_fail_on_scarb_version_lt_2_8_0() {

@PoulavBhowmick03
It seems that this comment is still unaddressed

ddoktorski · 2025-03-04T09:44:23Z

@PoulavBhowmick03
Are you planning to address the last comment and resolve merge conflicts? Once that’s done it should be good to go.

ksew1 · 2025-03-25T12:44:44Z

Hi @PoulavBhowmick03
Please address the last comment and resolve merge conflicts or i will need to close this pr :(

PoulavBhowmick03 · 2025-03-26T13:17:49Z

@ksew1 hello, i was actually going through a surgery, and so i was completely away from my works, give me time till this weekend, it will be ready

improved validation for scarb version and scarb.toml config

50bc824

PoulavBhowmick03 requested review from cptartur, ddoktorski, franciszekjob and kkawula as code owners January 30, 2025 19:08

ddoktorski requested changes Jan 31, 2025

View reviewed changes

PoulavBhowmick03 added 2 commits February 1, 2025 11:23

Merge branch 'master' of github.com:PoulavBhowmick03/starknet-foundry…

f26b46f

… into improve_scarb_validation

fixed the changes required in workspace.rs and function names

f25f823

PoulavBhowmick03 requested a review from ddoktorski February 1, 2025 05:55

ksew1 requested changes Feb 3, 2025

View reviewed changes

Merge branch 'master' of github.com:PoulavBhowmick03/starknet-foundry…

4d2b7f5

… into improve_scarb_validation

PoulavBhowmick03 added 2 commits February 10, 2025 02:03

Merge branch 'master' of github.com:PoulavBhowmick03/starknet-foundry…

322c7ae

… into improve_scarb_validation

modified the backtrace tests

15b5f4b

PoulavBhowmick03 requested a review from a team as a code owner February 10, 2025 03:57

PoulavBhowmick03 requested a review from ksew1 February 10, 2025 03:57

ksew1 requested changes Feb 10, 2025

View reviewed changes

.tool-versions Outdated Show resolved Hide resolved

PoulavBhowmick03 added 2 commits February 10, 2025 17:37

applied suggestions and changes

61c3eae

applied suggestions and changes

cd3e04a

ksew1 requested changes Feb 10, 2025

View reviewed changes

ran cargo fmt

6cdc3a3

ksew1 closed this Apr 10, 2025

	const BACKTRACE_REQUIRED_ENTRIES: [(&str, &str); 2] = [
	const BACKTRACE_REQUIRED_ENTRIES: [(&str,bool); 2] = [


		let manifest = fs::read_to_string(&scarb_metadata.runtime_manifest)?.parse::<DocumentMut>()?;

		let profile_cairo = manifest

	VersionedCoverageAnnotations::try_from_debug_info(sierra_debug_info).context(
	VersionedCoverageAnnotations::try_from_debug_info(sierra_debug_info)?;

	VersionedProfilerAnnotations::try_from_debug_info(sierra_debug_info).context(
	VersionedProfilerAnnotations::try_from_debug_info(sierra_debug_info)?;

improved validation for scarb version and scarb.toml config #2900

improved validation for scarb version and scarb.toml config #2900

Uh oh!

Conversation

PoulavBhowmick03 commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Introduced changes

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ksew1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksew1 Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddoktorski commented Feb 4, 2025

Uh oh!

ddoktorski commented Feb 6, 2025

Uh oh!

PoulavBhowmick03 commented Feb 7, 2025

Uh oh!

PoulavBhowmick03 commented Feb 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PoulavBhowmick03 commented Feb 10, 2025

Uh oh!

ksew1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddoktorski commented Mar 4, 2025

Uh oh!

ksew1 commented Mar 25, 2025

Uh oh!

PoulavBhowmick03 commented Mar 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PoulavBhowmick03 commented Jan 30, 2025 •

edited

Loading

ksew1 Feb 4, 2025 •

edited

Loading