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

Added corelib version validation when loading db. #6204

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Aug 13, 2024

This change is Reviewable

@orizi orizi requested a review from mkaput as a code owner August 13, 2024 08:28
@orizi orizi force-pushed the pr/orizi/dev-v2.7.1/9cabb842 branch from 6b77674 to 9823c3a Compare August 13, 2024 08:31
@orizi orizi linked an issue Aug 13, 2024 that may be closed by this pull request
@orizi orizi force-pushed the pr/orizi/dev-v2.7.1/9cabb842 branch from 9823c3a to 5bc9cb3 Compare August 13, 2024 08:37
@orizi orizi force-pushed the pr/orizi/dev-v2.7.1/9cabb842 branch from 5bc9cb3 to 5abac98 Compare August 13, 2024 08:57
Copy link
Collaborator

@maciektr maciektr left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 11 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware and @mkaput)

Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 11 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware, @mkaput, and @orizi)


crates/cairo-lang-filesystem/src/db.rs line 44 at r3 (raw file):

    pub edition: Edition,
    /// The crate's version.
    pub version: Option<Version>,

This field is ignored for everything but corelib. Can we instead change ProjectConfig to

pub struct ProjectConfig {
    // ...
    pub corelib: CorelibConfiguration,
   // ...
}

pub struct CorelibConfiguration {
    pub root: Option<Directory>,
    pub version: Version,  // or `Option<Version>`
}

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware, @mkaput, and @piotmag769)


crates/cairo-lang-filesystem/src/db.rs line 44 at r3 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

This field is ignored for everything but corelib. Can we instead change ProjectConfig to

pub struct ProjectConfig {
    // ...
    pub corelib: CorelibConfiguration,
   // ...
}

pub struct CorelibConfiguration {
    pub root: Option<Directory>,
    pub version: Version,  // or `Option<Version>`
}

currently only for it - but i see no good reason to actually separate.

Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 11 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware, @mkaput, and @orizi)


crates/cairo-lang-filesystem/src/db.rs line 44 at r3 (raw file):

Previously, orizi wrote…

currently only for it - but i see no good reason to actually separate.

What other crate versions would you want to validate in the compiler? I think everything else should be responsibility of a package manager (Scarb), it mixes abstraction layers

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware, @mkaput, and @piotmag769)


crates/cairo-lang-filesystem/src/db.rs line 44 at r3 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

What other crate versions would you want to validate in the compiler? I think everything else should be responsibility of a package manager (Scarb), it mixes abstraction layers

i don't know what i would have wanted to check - but this specific check is due to the setup possibly changing the configuration.

if i cannot take the version from a specific crate - but only from the corelib - i won't be capable of checking anything.

Additionally, in the future, when starknet crate is extracted from core - it would require similar checks.

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @mkaput, and @piotmag769)


crates/cairo-lang-compiler/src/db.rs line 169 at r3 (raw file):

        if let Some(config) = &self.project_config {
            update_crate_roots_from_project_config(&mut db, config.as_ref());
            if let Some(corelib) = &config.corelib {

@piotmag769 unless you claim the entire validation should only happen due to this setup.

if so - i don't really understand why this isn't being directly solved at scarb level.

Code quote:

            if let Some(corelib) = &config.corelib {

Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @mkaput)


crates/cairo-lang-filesystem/src/db.rs line 44 at r3 (raw file):

Previously, orizi wrote…

i don't know what i would have wanted to check - but this specific check is due to the setup possibly changing the configuration.

if i cannot take the version from a specific crate - but only from the corelib - i won't be capable of checking anything.

Additionally, in the future, when starknet crate is extracted from core - it would require similar checks.

Not a fan of that, but will unblock

Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware, @mkaput, and @orizi)


crates/cairo-lang-compiler/src/db.rs line 169 at r3 (raw file):

Previously, orizi wrote…

@piotmag769 unless you claim the entire validation should only happen due to this setup.

if so - i don't really understand why this isn't being directly solved at scarb level.

This one is solved at Scarb level, so I didn't mean that

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

:lgtm:

Reviewed 11 of 11 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkaput and @orizi)

@orizi orizi added this pull request to the merge queue Aug 13, 2024
Merged via the queue into dev-v2.7.1 with commit 925afea Aug 13, 2024
87 checks passed
@orizi orizi deleted the pr/orizi/dev-v2.7.1/9cabb842 branch August 19, 2024 06:52
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.

Deal with panics on core crate mismatch
4 participants