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 cairo-lang-primitive-token from registry not path in unreleased cairo crates #7236

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

maciektr
Copy link
Collaborator

@maciektr maciektr commented Feb 7, 2025

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@maciektr maciektr marked this pull request as ready for review February 7, 2025 12:22
@maciektr maciektr requested review from mkaput and orizi February 7, 2025 12:22
Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @maciektr and @orizi)


a discussion (no related file):
what's the reasoning behind this change?

@maciektr
Copy link
Collaborator Author

maciektr commented Feb 7, 2025

Previously, mkaput (Marek Kaput) wrote…

what's the reasoning behind this change?

Without this change, when you depend on the parser through git dependency, transitive dependency to cairo-lang-primitive-token will be translated into local git dependency, not the remote registry one. In that case, Rustc is unable to determine that both of this crates (local git and registry remote) are identical and fails with an error:

   Compiling some v1.0.0 ([..]Scarb.toml)
error[E0271]: expected `SyntaxNodeWithDbIterator<'_, SimpleParserDatabase>` to be an iterator that yields `PrimitiveToken`, but it yields `PrimitiveToken`
   --> src/lib.rs:45:16
    |
45  |     let tokens = quote! {
    |  ________________^
46  | |     #syntax_node_with_db
47  | |     trait Circle {
48  | |       fn print() -> ();
...   |
54  | |     }
55  | |   };
    | |___^ expected `PrimitiveToken`, found a different `PrimitiveToken`
    |
    = note: `PrimitiveToken` and `PrimitiveToken` have similar names, but are actually distinct types

@maciektr maciektr requested a review from mkaput February 7, 2025 14:04
Copy link
Member

@mkaput mkaput 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 @maciektr and @orizi)


a discussion (no related file):

Previously, maciektr (maciektr) wrote…

Without this change, when you depend on the parser through git dependency, transitive dependency to cairo-lang-primitive-token will be translated into local git dependency, not the remote registry one. In that case, Rustc is unable to determine that both of this crates (local git and registry remote) are identical and fails with an error:

   Compiling some v1.0.0 ([..]Scarb.toml)
error[E0271]: expected `SyntaxNodeWithDbIterator<'_, SimpleParserDatabase>` to be an iterator that yields `PrimitiveToken`, but it yields `PrimitiveToken`
   --> src/lib.rs:45:16
    |
45  |     let tokens = quote! {
    |  ________________^
46  | |     #syntax_node_with_db
47  | |     trait Circle {
48  | |       fn print() -> ();
...   |
54  | |     }
55  | |   };
    | |___^ expected `PrimitiveToken`, found a different `PrimitiveToken`
    |
    = note: `PrimitiveToken` and `PrimitiveToken` have similar names, but are actually distinct types

Can you provide this explanation as a comment here? It's surprising that local crate is not used via local path. 🙏

@maciektr maciektr force-pushed the maciektr/primitive-token branch from 1618ee8 to 8317e34 Compare February 7, 2025 14:33
@maciektr maciektr requested a review from mkaput February 7, 2025 14:33
Copy link
Collaborator Author

@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.

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @mkaput and @orizi)


a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

Can you provide this explanation as a comment here? It's surprising that local crate is not used via local path. 🙏

Done.

Copy link
Collaborator

@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.

Reviewed all commit messages.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @mkaput)


a discussion (no related file):

Previously, maciektr (maciektr) wrote…

Done.

this sounds like we should just move this crate out of the crate tree here completely - and ONLY depend on the registry one.

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @maciektr)


a discussion (no related file):

Previously, orizi wrote…

this sounds like we should just move this crate out of the crate tree here completely - and ONLY depend on the registry one.

Makes sense, although who should host it? I think the repo should be in starkware-libs, to minimize dependencies on SWM repos here.

@maciektr
Copy link
Collaborator Author

Previously, mkaput (Marek Kaput) wrote…

Makes sense, although who should host it? I think the repo should be in starkware-libs, to minimize dependencies on SWM repos here.

@orizi do you have an opinion here? 🤔

@maciektr maciektr force-pushed the maciektr/primitive-token branch from 8317e34 to b5fdf14 Compare February 11, 2025 16:19
Copy link
Collaborator

@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 @maciektr)


a discussion (no related file):

Previously, maciektr (maciektr) wrote…

@orizi do you have an opinion here? 🤔

we can have it in the repo - just not as a part of the workspace.

@maciektr maciektr force-pushed the maciektr/primitive-token branch 3 times, most recently from 9e0a922 to 81c87f6 Compare February 11, 2025 18:51
@maciektr maciektr requested a review from orizi February 11, 2025 19:06
@maciektr
Copy link
Collaborator Author

Previously, orizi wrote…

we can have it in the repo - just not as a part of the workspace.

Removed from workspace

Copy link
Collaborator

@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.

Reviewed 2 of 2 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @maciektr)


crates/cairo-lang-parser/Cargo.toml line 14 at r3 (raw file):

# Registry dep is used instead of local path, so the same exact source can be used by crates
# depending on this crate, from both registry and git.
cairo-lang-primitive-token = "1"

move dep and doc to workspace.

Suggestion:

cairo-lang-primitive-token.worksspace = true

crates/cairo-lang-syntax/Cargo.toml line 15 at r3 (raw file):

# Registry dep is used instead of local path, so the same exact source can be used by crates
# depending on this crate, from both registry and git.
cairo-lang-primitive-token = "1"

move dep and doc to workspace.

Suggestion:

cairo-lang-primitive-token.worksspace = true

@maciektr maciektr force-pushed the maciektr/primitive-token branch from 81c87f6 to ccc6e7a Compare February 12, 2025 09:10
Copy link
Collaborator Author

@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.

Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @mkaput and @orizi)


crates/cairo-lang-syntax/Cargo.toml line 15 at r3 (raw file):

Previously, orizi wrote…

move dep and doc to workspace.

Done.

Copy link
Collaborator Author

@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.

Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @mkaput and @orizi)


crates/cairo-lang-parser/Cargo.toml line 14 at r3 (raw file):

Previously, orizi wrote…

move dep and doc to workspace.

Done.

@maciektr maciektr requested a review from orizi February 12, 2025 09:11
Copy link
Collaborator

@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.

:lgtm:

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @maciektr)

Copy link
Collaborator

@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 @maciektr)

@maciektr maciektr enabled auto-merge February 12, 2025 10:01
@maciektr maciektr added this pull request to the merge queue Feb 12, 2025
Merged via the queue into main with commit 66f5c72 Feb 12, 2025
47 checks passed
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