Add component dependencies to metadata #1655
Conversation
piotmag769
requested review from
maciektr,
DelevoXDG and
wawel37
as code owners
| pub fn to_discriminator(&self) -> String { | ||
| self.package_id.to_serialized_string() | ||
| } |
There was a problem hiding this comment.
Changed discriminator to be serialized PackageId instead of Version to be aligned with the comment regarding CompilationUnitComponentDependencyMetadata::id
There was a problem hiding this comment.
This requires changes to Cairo to work ^
There was a problem hiding this comment.
This requires changes to Cairo to work ^
WDYM?
There was a problem hiding this comment.
I mean ProjectConfig is a terrible structure that sometimes has duplicated information, but sometimes lacks it (like in this case), so rn in RootDatabaseBuilder we set the discriminator to always be equal to CrateSetting.version
| pub fn to_discriminator(&self) -> String { | ||
| self.package_id.to_serialized_string() | ||
| } |
There was a problem hiding this comment.
This requires changes to Cairo to work ^
WDYM?
| ( | ||
| compilation_unit_component.package.id.name.to_string(), | ||
| DependencySettings { | ||
| discriminator: (compilation_unit_component.package.id.name.to_string() | ||
| != *CORELIB_CRATE_NAME) | ||
| .then_some(compilation_unit_component.package.id.version.clone()) | ||
| .map(|v| v.to_smolstr()), | ||
| .then_some(compilation_unit_component.id.clone()) |
There was a problem hiding this comment.
I wonder if we should hide the corelib edge case in cu too.
There was a problem hiding this comment.
Definitely. I don't think we want all tools to think about this case
There was a problem hiding this comment.
To be fair it doesn't even matter from the compiler's point of view if the corelib discriminator is None or not as long as core is a dependency for each package (which it is in Scarb)
I agree though that we should keep it as None for consistency, will change it in metadata and Scarb's internal model
There was a problem hiding this comment.
Are you sure it does not matter? I know this has not been working without this if when we first introduced it. If we can omit this logic, it would be best to do so I think 🤔
There was a problem hiding this comment.
It does matter because compiler has this assumption: https://github.com/starkware-libs/cairo/blob/5900876001c7d71008cc3d556842c081bc57b094/crates/cairo-lang-filesystem/src/ids.rs#L39-L42
No description provided.