Treat Cairo plugins as package dependencies #1889
Conversation
| /// The ID of the [`CompilationUnitComponent`] the plugin is represented by in the [`CairoCompilationUnit`]. | ||
| pub discriminator: CompilationUnitComponentId, |
There was a problem hiding this comment.
This is unnecessary, you don't need to keep it here. You can just use the package.id.to_serialized_string in metadata builder
scarb/src/ops/metadata.rs
Outdated
| @@ -243,6 +243,9 @@ fn collect_cairo_compilation_unit_metadata( | |||
| .map(|c| { | |||
| m::CompilationUnitCairoPluginMetadataBuilder::default() | |||
| .package(wrap_package_id(c.package.id)) | |||
| .discriminator(m::CompilationUnitComponentId { | |||
| repr: c.discriminator.package_id.to_string(), | |||
There was a problem hiding this comment.
scarb/scarb/src/ops/metadata.rs
Line 401 in 6769c8b
| repr: c.discriminator.package_id.to_string(), | |
| repr: c.discriminator.package_id.to_serialized_string(), |
Also not needed here, check comment above
| /// An ID which uniquely identifies the plugin in scope of the Compilation Unit. | ||
| pub discriminator: Option<CompilationUnitComponentId>, |
There was a problem hiding this comment.
| /// An ID which uniquely identifies the plugin in scope of the Compilation Unit. | |
| pub discriminator: Option<CompilationUnitComponentId>, | |
| /// An id which uniquely identifies the plugin in scope of the compilation unit amongst other plugins and [`CompilationUnitComponent`]s. It is used to identify the plugin as a possible dependency of a [`CompilationUnitComponent`]. | |
| pub component_dependency_id: Option<CompilationUnitComponentId>, |
Please check other related docs as well (e.g. docs for CompilationUnitComponent.dependencies and CompilationUnitComponent.id) and update them to reflect the new semantics
scarb/src/ops/resolve.rs
Outdated
| let member_plugin_dependencies = cairo_plugins | ||
| .iter() | ||
| .map(|plugin| plugin.discriminator.clone()); | ||
|
|
||
| let member_component = components | ||
| .iter_mut() | ||
| .find(|component| component.package.id == member.id) | ||
| .unwrap(); | ||
|
|
||
| member_component | ||
| .dependencies | ||
| .extend(member_plugin_dependencies); | ||
|
|
There was a problem hiding this comment.
I don't understand where it comes from. Isn't adding the plugin deps in component_dependencies enough?
scarb/src/ops/resolve.rs
Outdated
| self.resolve | ||
| .package_dependencies(package_id, &TargetKind::CAIRO_PLUGIN) | ||
| }; |
There was a problem hiding this comment.
It doesn't treat the plugins globally (i.e. it treats packages like humanlike dependencies), does it?
| if unit.cairo_plugins | ||
| .iter() | ||
| .find(|plugin| plugin.discriminator == *compilation_unit_component_id) | ||
| .map(|plugin| (plugin.package.id.name.to_string(), plugin.discriminator.clone())) | ||
| .is_some() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Why do you filter them out?
integraledelebesgue
force-pushed
the
spr/main/52f11b96
branch
from
5ecc4f8 to
f9afa40
Compare
commit-id:52f11b96
integraledelebesgue
force-pushed
the
spr/main/52f11b96
branch
from
f9afa40 to
f4deb31
Compare
| if unit.cairo_plugins | ||
| .iter() | ||
| .find(|plugin| plugin.discriminator == *compilation_unit_component_id) | ||
| .map(|plugin| (plugin.package.id.name.to_string(), plugin.discriminator.clone())) |
There was a problem hiding this comment.
Why you need this .map if you check for Some only?
Stack: