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

Add component dependencies to metadata #1655

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 52 additions & 6 deletions scarb-metadata/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
#![deny(rustdoc::private_intra_doc_links)]
#![warn(rust_2018_idioms)]
#![doc = concat!(
"Structured access to the output of `scarb metadata --format-version ",
env!("CARGO_PKG_VERSION_MAJOR"),
"`.
"Structured access to the output of `scarb metadata --format-version ",
env!("CARGO_PKG_VERSION_MAJOR"),
"`.
piotmag769 marked this conversation as resolved.
Show resolved Hide resolved
")]
//! Usually used by Scarb extensions and other developer tools.
//!
Expand Down Expand Up @@ -37,7 +37,7 @@ mod command;
mod version_pin;

/// An "opaque" identifier for a package.
/// It is possible to inspect the `repr` field, if the need arises,
/// It is possible to inspect the `repr` field if the need arises,
/// but its precise format is an implementation detail and is subject to change.
///
/// [`Metadata`] can be indexed by [`PackageId`].
Expand All @@ -61,7 +61,7 @@ impl fmt::Display for PackageId {
}

/// An "opaque" identifier for a source.
/// It is possible to inspect the `repr` field, if the need arises,
/// It is possible to inspect the `repr` field if the need arises,
/// but its precise format is an implementation detail and is subject to change.
#[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[serde(transparent)]
Expand All @@ -83,7 +83,7 @@ impl fmt::Display for SourceId {
}

/// An "opaque" identifier for a compilation unit.
/// It is possible to inspect the `repr` field, if the need arises,
/// It is possible to inspect the `repr` field if the need arises,
/// but its precise format is an implementation detail and is subject to change.
///
/// [`Metadata`] can be indexed by [`CompilationUnitId`].
Expand All @@ -106,6 +106,30 @@ impl fmt::Display for CompilationUnitId {
}
}

/// An "opaque" identifier for a compilation unit component.
/// It is possible to inspect the `repr` field if the need arises,
/// but its precise format is an implementation detail and is subject to change.
///
/// [`Metadata`] can be indexed by [`CompilationUnitComponentId`].
mkaput marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[serde(transparent)]
pub struct CompilationUnitComponentId {
/// The underlying string representation of the ID.
pub repr: String,
}

impl From<String> for CompilationUnitComponentId {
fn from(repr: String) -> Self {
Self { repr }
}
}

impl fmt::Display for CompilationUnitComponentId {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Display::fmt(&self.repr, f)
}
}

fn current_profile_default() -> String {
"release".to_string()
}
Expand Down Expand Up @@ -356,6 +380,28 @@ pub struct CompilationUnitComponentMetadata {
/// If not specified, the one from `CompilationUnit` will be used.
#[serde(default)]
pub cfg: Option<Vec<Cfg>>,
/// Identifier of this component. It is unique in its compilation unit.
pub id: Option<CompilationUnitComponentId>,
/// Dependencies of this component.
pub dependencies: Option<Vec<CompilationUnitComponentDependencyMetadata>>,
maciektr marked this conversation as resolved.
Show resolved Hide resolved

/// Additional data not captured by deserializer.
#[cfg_attr(feature = "builder", builder(default))]
#[serde(flatten)]
pub extra: HashMap<String, serde_json::Value>,
}

/// Information about dependency of a component of a compilation unit.
#[derive(Clone, Serialize, Deserialize, Debug, Eq, PartialEq)]
#[cfg_attr(feature = "builder", derive(Builder))]
#[cfg_attr(feature = "builder", builder(setter(into)))]
#[non_exhaustive]
pub struct CompilationUnitComponentDependencyMetadata {
/// Id of a component from the same compilation unit that this dependency refers to.
///
/// This should directly translate to a `discriminator` field in Cairo compiler terminology,
/// except that it should be `None` for `core` crate **only**.
pub id: CompilationUnitComponentId,

/// Additional data not captured by deserializer.
#[cfg_attr(feature = "builder", builder(default))]
Expand Down
22 changes: 22 additions & 0 deletions scarb/src/compiler/compilation_unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,16 @@ pub struct ProcMacroCompilationUnit {
#[derive(Clone, Debug)]
#[non_exhaustive]
pub struct CompilationUnitComponent {
/// Unique id identifying this component.
pub id: CompilationUnitComponentId,
/// The Scarb [`Package`] to be built.
pub package: Package,
/// Information about the specific target to build, out of the possible targets in `package`.
pub targets: Vec<Target>,
/// Items for the Cairo's `#[cfg(...)]` attribute to be enabled in this component.
pub cfg_set: Option<CfgSet>,
/// Dependencies of this component.
pub dependencies: Vec<CompilationUnitComponentId>,
}

/// Information about a single package that is a compiler plugin to load for [`CompilationUnit`].
Expand All @@ -92,6 +96,20 @@ pub struct CompilationUnitCairoPlugin {
pub builtin: bool,
}

/// Unique identifier of the compilation unit component.
/// Currently, a compilation unit can be uniquely identified by [`PackageId`] only.
/// It may be not sufficient in the future depending on changes to the compilation model.
#[derive(Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)]
pub struct CompilationUnitComponentId {
pub package_id: PackageId,
}

impl CompilationUnitComponentId {
pub fn to_discriminator(&self) -> String {
self.package_id.to_serialized_string()
}
Comment on lines +108 to +110
Copy link
Contributor Author

@piotmag769 piotmag769 Oct 17, 2024

Choose a reason for hiding this comment

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

Changed discriminator to be serialized PackageId instead of Version to be aligned with the comment regarding CompilationUnitComponentDependencyMetadata::id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This requires changes to Cairo to work ^

Copy link
Contributor

Choose a reason for hiding this comment

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

This requires changes to Cairo to work ^

WDYM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

https://github.com/starkware-libs/cairo/blob/932767340c4b8a762140bf9eba305f437587ac1b/crates/cairo-lang-compiler/src/project.rs#L148-L152

}

pub trait CompilationUnitAttributes {
fn main_package_id(&self) -> PackageId;
fn components(&self) -> &[CompilationUnitComponent];
Expand Down Expand Up @@ -288,9 +306,13 @@ impl CompilationUnitComponent {
);
}
Ok(Self {
id: CompilationUnitComponentId {
package_id: package.id,
},
package,
targets,
cfg_set,
dependencies: vec![],
})
}

Expand Down
57 changes: 8 additions & 49 deletions scarb/src/compiler/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use tracing::trace;

use crate::compiler::plugin::proc_macro::{ProcMacroHost, ProcMacroHostPlugin};
use crate::compiler::{CairoCompilationUnit, CompilationUnitAttributes, CompilationUnitComponent};
use crate::core::{ManifestDependency, TestTargetProps, TestTargetType, Workspace};
use crate::core::Workspace;
use crate::DEFAULT_MODULE_MAIN_FILE;

pub struct ScarbDatabase {
Expand Down Expand Up @@ -145,66 +145,25 @@ fn build_project_config(unit: &CairoCompilationUnit) -> Result<ProjectConfig> {
.map(|component| {
let experimental_features = component.package.manifest.experimental_features.clone();
let experimental_features = experimental_features.unwrap_or_default();
// Those are direct dependencies of the component.
maciektr marked this conversation as resolved.
Show resolved Hide resolved
let dependencies_summary: Vec<&ManifestDependency> = component
.package
.manifest
.summary
.full_dependencies()
.collect();

// We iterate over all of the compilation unit components to get dependency's version.
let mut dependencies: BTreeMap<String, DependencySettings> = unit
.components
let dependencies: BTreeMap<String, DependencySettings> = component
.dependencies
.iter()
.filter(|component_as_dependency| {
dependencies_summary.iter().any(|dependency_summary| {
dependency_summary.name == component_as_dependency.package.id.name
}) ||
// This is a hacky way of accommodating integration test components,
// which need to depend on the tested package.
component_as_dependency
.package
.manifest
.targets
.iter()
.filter(|target| target.kind.is_test())
.any(|target| {
target.group_id.clone().unwrap_or(target.name.clone())
== component.package.id.name.to_smol_str()
&& component_as_dependency.cairo_package_name() != component.cairo_package_name()
})
})
.map(|compilation_unit_component| {
.map(|compilation_unit_component_id| {
let compilation_unit_component = unit.components.iter().find(|component| component.id == *compilation_unit_component_id)
.expect("Dependency of a component is guaranteed to exist in compilation unit components");
piotmag769 marked this conversation as resolved.
Show resolved Hide resolved
(
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())
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should hide the corelib edge case in cu too.

Copy link
Member

Choose a reason for hiding this comment

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

Definitely. I don't think we want all tools to think about this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

https://github.com/starkware-libs/cairo/blob/932767340c4b8a762140bf9eba305f437587ac1b/crates/cairo-lang-semantic/src/resolve/mod.rs#L1077

I agree though that we should keep it as None for consistency, will change it in metadata and Scarb's internal model

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member

Choose a reason for hiding this comment

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

.map(|v| v.to_discriminator().to_smolstr()),
},
)
})
.collect();

// Adds itself to dependencies
let is_integration_test = if component.first_target().kind.is_test() {
let props: Option<TestTargetProps> = component.first_target().props().ok();
props
.map(|props| props.test_type == TestTargetType::Integration)
.unwrap_or_default()
} else { false };
if !is_integration_test {
dependencies.insert(
component.package.id.name.to_string(),
DependencySettings {
discriminator: (component.package.id.name.to_string() != *CORELIB_CRATE_NAME)
.then_some(component.package.id.version.clone()).map(|v| v.to_smolstr()),
},
);
}

(
component.cairo_package_name(),
CrateSettings {
Expand Down
12 changes: 12 additions & 0 deletions scarb/src/ops/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,18 @@ where
.expect("Cairo's `Cfg` must serialize identically as Scarb Metadata's `Cfg`.")
})
.collect::<Vec<_>>()))
.dependencies(
Some(
c.dependencies
.iter()
.map(|component_id|
m::CompilationUnitComponentDependencyMetadataBuilder::default()
.id(component_id.to_discriminator())
.build().
unwrap()
).collect()
)
)
.build()
.unwrap()
})
Expand Down
58 changes: 57 additions & 1 deletion scarb/src/ops/resolve.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::compiler::plugin::{fetch_cairo_plugin, CairoPluginProps};
use crate::compiler::{
CairoCompilationUnit, CompilationUnit, CompilationUnitAttributes, CompilationUnitCairoPlugin,
CompilationUnitComponent, ProcMacroCompilationUnit, Profile,
CompilationUnitComponent, CompilationUnitComponentId, ProcMacroCompilationUnit, Profile,
};
use crate::core::lockfile::Lockfile;
use crate::core::package::{Package, PackageClass, PackageId};
Expand All @@ -26,6 +26,7 @@ use futures::TryFutureExt;
use indoc::formatdoc;
use itertools::Itertools;
use std::collections::{BTreeMap, HashMap, HashSet, VecDeque};
use std::iter::zip;

pub struct WorkspaceResolve {
pub resolve: Resolve,
Expand Down Expand Up @@ -390,6 +391,61 @@ fn cairo_compilation_unit_for_target(
member.id
};

// Collect dependencies for the components.
maciektr marked this conversation as resolved.
Show resolved Hide resolved
let dependencies_for_components: Vec<_> = components
.iter()
.map(|component| {
// Those are direct dependencies of the component.
let dependencies_summary: Vec<&ManifestDependency> = component
.package
.manifest
.summary
.full_dependencies()
.collect();

// We iterate over all the compilation unit components to get dependency's version.
let mut dependencies: Vec<CompilationUnitComponentId> = components
.iter()
.filter(|component_as_dependency| {
dependencies_summary.iter().any(|dependency_summary| {
dependency_summary.name == component_as_dependency.package.id.name
}) ||
// This is a hacky way of accommodating integration test components,
// which need to depend on the tested package.
component_as_dependency
.package
.manifest
.targets
.iter()
.filter(|target| target.kind.is_test())
.any(|target| {
target.group_id.clone().unwrap_or(target.name.clone())
== component.package.id.name.to_smol_str()
&& component_as_dependency.cairo_package_name() != component.cairo_package_name()
})
})
.map(|compilation_unit_component| compilation_unit_component.id.clone()
)
.collect();

// Adds itself to dependencies
let is_integration_test = if component.first_target().kind.is_test() {
let props: Option<TestTargetProps> = component.first_target().props().ok();
props
.map(|props| props.test_type == TestTargetType::Integration)
.unwrap_or_default()
} else { false };
if !is_integration_test {
dependencies.push(component.id.clone());
}

dependencies
}).collect();

for (component, dependencies) in zip(&mut components, dependencies_for_components) {
component.dependencies = dependencies;
}

Ok(CairoCompilationUnit {
main_package_id,
components,
Expand Down
Loading