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

Adjust ProjectConfig to remove implicit assumption that discriminator == version #6503

Closed

Conversation

piotmag769
Copy link
Collaborator

Currently there is no way to specify the discriminator for a crate itself, so we do a wild guess and assume that it is equal to version. I also removed the ProjectConfig.corelib field that seems obsolete: API user can pass this information via ProjectConfigContent.content_roots.

@piotmag769 piotmag769 linked an issue Oct 22, 2024 that may be closed by this pull request
@reviewable-StarkWare
Copy link

This change is Reviewable

@piotmag769 piotmag769 marked this pull request as ready for review October 22, 2024 01:25
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 9 files at r2, all commit messages.
Reviewable status: 2 of 10 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @maciektr, @mkaput, and @piotmag769)


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

/// This directly translates to [`DependencySettings.discriminator`] expect the discriminator
/// **must** be `None` for the core crate.
pub type CrateIdentifier = SmolStr;

either do a type wrapper - or remove the alias.
`pub struct CrateIdentifier(SmolStr);

Code quote:

pub type CrateIdentifier = SmolStr;

Copy link
Contributor

@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: 2 of 10 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @maciektr, and @piotmag769)

a discussion (no related file):
I don't understand these changes. Why do you need these?

As far as I know things, the discriminator == version equality is used because this is a model that cairo_project.toml uses. There's open path to skip this requirement by directly interning things in *Database, which we do everywhere we want to, don't we?


Copy link
Contributor

@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: 2 of 10 files reviewed, 3 unresolved discussions (waiting on @Arcticae, @Draggu, @maciektr, and @piotmag769)

a discussion (no related file):
You're mixing two changes in a single PR here, please make a stack


Copy link
Contributor

@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: 2 of 10 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @maciektr, and @piotmag769)

a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

I don't understand these changes. Why do you need these?

As far as I know things, the discriminator == version equality is used because this is a model that cairo_project.toml uses. There's open path to skip this requirement by directly interning things in *Database, which we do everywhere we want to, don't we?

discussed offline. we need these changes anyway for scarb eject to be possible


Copy link
Collaborator

@Draggu Draggu 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 1 of 7 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @maciektr, and @piotmag769)

Copy link
Collaborator Author

@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 1 of 7 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @maciektr, and @orizi)


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

Previously, orizi wrote…

either do a type wrapper - or remove the alias.
`pub struct CrateIdentifier(SmolStr);

Done.

Copy link
Collaborator Author

@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 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @maciektr, and @orizi)

Copy link
Collaborator Author

@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 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @maciektr, @mkaput, and @orizi)

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: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @maciektr, @mkaput, and @piotmag769)

@piotmag769 piotmag769 force-pushed the 6480-pull-crate-dependencies-settings-from-scarb branch 3 times, most recently from 81224a5 to 3119a09 Compare October 23, 2024 14:43
@piotmag769 piotmag769 force-pushed the 6480-pull-crate-dependencies-settings-from-scarb branch from 3119a09 to 745d0c6 Compare October 23, 2024 14:45
@piotmag769 piotmag769 closed this Oct 23, 2024
@piotmag769
Copy link
Collaborator Author

Closing in favour of PR stack: #6511 #6512

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.

Pull crate dependencies settings from scarb
5 participants