-
Notifications
You must be signed in to change notification settings - Fork 70
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
Load prebuilt macros #1812
Load prebuilt macros #1812
Changes from all commits
b35f947
908cc47
7d399d3
a95a25d
2aa6ee8
bfeb684
e570a99
c73c0f6
87f87c5
a4a8d14
c81d9ce
9cac622
eba6fe4
57cc3d1
6b342b1
e76fcf2
8424cb6
be7b5c2
1e2845a
8a468d3
394bf55
7b0162c
76118b0
05cf39d
5a85ee2
cce42b7
06aab97
64f362e
e1873fd
034af9a
b8c4156
3af99af
9eb6fc4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,16 @@ | ||
use std::fmt::Write; | ||
use std::hash::{Hash, Hasher}; | ||
|
||
use anyhow::{ensure, Result}; | ||
use cairo_lang_filesystem::cfg::CfgSet; | ||
use cairo_lang_filesystem::db::CrateIdentifier; | ||
use itertools::Itertools; | ||
use serde::{Deserialize, Serialize}; | ||
use smol_str::SmolStr; | ||
use std::fmt::Write; | ||
use std::hash::{Hash, Hasher}; | ||
use std::sync::Arc; | ||
use typed_builder::TypedBuilder; | ||
|
||
use crate::compiler::plugin::proc_macro::compilation::SharedLibraryProvider; | ||
use crate::compiler::plugin::proc_macro::ProcMacroInstance; | ||
use crate::compiler::Profile; | ||
use crate::core::{ | ||
ManifestCompilerConfig, Package, PackageId, PackageName, Target, TargetKind, Workspace, | ||
|
@@ -72,6 +74,9 @@ pub struct ProcMacroCompilationUnit { | |
|
||
/// Rust compiler configuration parameters to use in this unit. | ||
pub compiler_config: serde_json::Value, | ||
|
||
/// Instance of the proc macro loaded from prebuilt library, if available. | ||
pub prebuilt: Option<Arc<ProcMacroInstance>>, | ||
} | ||
|
||
/// Information about a single package that is part of a [`CompilationUnit`]. | ||
|
@@ -97,6 +102,9 @@ pub struct CompilationUnitCairoPlugin { | |
/// The Scarb plugin [`Package`] to load. | ||
pub package: Package, | ||
pub builtin: bool, | ||
|
||
/// Instance of the proc macro loaded from prebuilt library, if available. | ||
pub prebuilt: Option<Arc<ProcMacroInstance>>, | ||
} | ||
|
||
/// Unique identifier of the compilation unit component. | ||
|
@@ -130,6 +138,8 @@ pub trait CompilationUnitAttributes { | |
fn components(&self) -> &[CompilationUnitComponent]; | ||
fn digest(&self) -> String; | ||
|
||
fn is_prebuilt(&self) -> bool; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that's the right place to store this information. In this PR you should load all available pre-builts, and we will add a whitelist later on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I understand 😅 This PR does load all available prebuilts, but we can't load any binaries if none are present for the target triple, even if they're included in the whitelist. This method merely indicated whether a proc macro has prebuilts for the current platform There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The trait |
||
fn main_component(&self) -> &CompilationUnitComponent { | ||
// NOTE: This uses the order invariant of `component` field. | ||
let component = &self.components()[0]; | ||
|
@@ -195,6 +205,12 @@ impl CompilationUnitAttributes for CompilationUnit { | |
Self::ProcMacro(unit) => unit.digest(), | ||
} | ||
} | ||
fn is_prebuilt(&self) -> bool { | ||
match self { | ||
Self::Cairo(unit) => unit.is_prebuilt(), | ||
Self::ProcMacro(unit) => unit.is_prebuilt(), | ||
} | ||
} | ||
} | ||
|
||
impl CompilationUnitAttributes for CairoCompilationUnit { | ||
|
@@ -215,6 +231,10 @@ impl CompilationUnitAttributes for CairoCompilationUnit { | |
self.compiler_config.hash(&mut hasher); | ||
hasher.finish_as_short_hash() | ||
} | ||
|
||
fn is_prebuilt(&self) -> bool { | ||
false | ||
} | ||
} | ||
|
||
impl CompilationUnitAttributes for ProcMacroCompilationUnit { | ||
|
@@ -233,6 +253,13 @@ impl CompilationUnitAttributes for ProcMacroCompilationUnit { | |
} | ||
hasher.finish_as_short_hash() | ||
} | ||
|
||
fn is_prebuilt(&self) -> bool { | ||
self.components | ||
.first() | ||
.map(|c| c.package.is_prebuilt()) | ||
.unwrap_or(false) | ||
} | ||
} | ||
Comment on lines
+257
to
+262
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be just |
||
|
||
impl CairoCompilationUnit { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ use itertools::Itertools; | |
use serde::{Deserialize, Serialize}; | ||
|
||
use crate::compiler::plugin::builtin::BuiltinCairoRunPlugin; | ||
use crate::compiler::plugin::proc_macro::compilation::SharedLibraryProvider; | ||
use crate::core::{Package, PackageId, TargetKind, Workspace}; | ||
|
||
use self::builtin::{BuiltinStarkNetPlugin, BuiltinTestPlugin}; | ||
|
@@ -29,8 +30,8 @@ pub fn fetch_cairo_plugin(package: &Package, ws: &Workspace<'_>) -> Result<()> { | |
assert!(package.is_cairo_plugin()); | ||
let target = package.fetch_target(&TargetKind::CAIRO_PLUGIN)?; | ||
let props: CairoPluginProps = target.props()?; | ||
// No need to fetch for buildin plugins. | ||
if !props.builtin { | ||
// No need to fetch for builtin or prebuilt plugins. | ||
if !props.builtin && !package.is_prebuilt() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment saying, it will not run fetch for plugins with invalid prebuilts, even though they will need to compile with Cargo before being used. |
||
proc_macro::fetch_crate(package, ws)?; | ||
} | ||
Ok(()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,12 +15,14 @@ use ra_ap_toolchain::Tool; | |
use scarb_ui::{Message, OutputFormat}; | ||
use serde::{Serialize, Serializer}; | ||
use serde_json::value::RawValue; | ||
use std::env::consts::DLL_SUFFIX; | ||
use std::fmt::Display; | ||
use std::fs; | ||
use std::io::{Seek, SeekFrom}; | ||
use std::ops::Deref; | ||
use std::process::Command; | ||
use tar::Archive; | ||
use target_triple::target; | ||
use tracing::trace_span; | ||
|
||
pub const PROC_MACRO_BUILD_PROFILE: &str = "release"; | ||
|
@@ -31,6 +33,10 @@ pub trait SharedLibraryProvider { | |
fn target_path(&self, config: &Config) -> Filesystem; | ||
/// Location of the shared library for the package. | ||
fn shared_lib_path(&self, config: &Config) -> Result<Utf8PathBuf>; | ||
/// Location of the prebuilt binary for the package. | ||
fn prebuilt_lib_path(&self) -> Result<Utf8PathBuf>; | ||
/// Returns true if the package contains a prebuilt binary. | ||
fn is_prebuilt(&self) -> bool; | ||
} | ||
|
||
impl SharedLibraryProvider for Package { | ||
|
@@ -61,6 +67,34 @@ impl SharedLibraryProvider for Package { | |
.path_unchecked() | ||
.join(lib_name)) | ||
} | ||
|
||
fn prebuilt_lib_path(&self) -> Result<Utf8PathBuf> { | ||
let target_triple = target!(); | ||
|
||
let prebuilt_name = format!( | ||
"{name}_v{version}_{target}{suffix}", | ||
name = self.id.name, | ||
version = self.id.version, | ||
target = target_triple, | ||
suffix = DLL_SUFFIX | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've considered this as well. Unfortunately, it will lead to naming conflicts, as for example library with a name Therefore I used format as described in the issue:
On the second look though
we may consider dropping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just meant replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It also adds What do you think about dropping the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is what Cargo compiles this packages into. Forcing macro authors to remove the prefix from compiled file is imho more confusing, that sticking to the conventional naming.
What do you mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I’m referring to the reason behind this warning. When naming build artifacts, Cargo uses the package name specified in the Cargo manifest, which:
The problem with 1. is that users will have to name packages manually, leading to confusion about whether the package name from the Cargo or Scarb manifest should be used - mistakes wills surely be made.
While it’s true that Cargo compiles libraries with a prefix, the resulting libraries must be renamed anyway to avoid conflicts:
Since renaming is inevitable, I argue that the simplest and most consistent naming convention - requiring minimal amount of documentation reading from the user - is:
This approach seems preferable to me alternatives like:
or:
What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
True! Sounds like we should turn this warning into a hard error soon, to warn macro authors early on about the mismatch 😄
Makes sense. So let's drop the prefix indeed. I would still keep the name + version, just to make sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍🏻
The current version does so, but I feel strongly that we should drop the name and version as well. They're redundant since this info is already available in the manifest, tarball name, and elsewhere, which potentially just makes things just a bit harder fot the macro developers. The only potential benefit I see is reducing the likelihood of outdated binaries being included. Unless you have any other reasons to keep it, I don't think this downside is really that important and justifies the trade-off. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It just makes it less likely to ship wrong version if some build script fails, so it's an advantage for me. I don't think adding this is really a problem. |
||
); | ||
|
||
let prebuilt_path = self | ||
.root() | ||
.join("target") | ||
.join("scarb") | ||
.join("cairo-plugin") | ||
.join(prebuilt_name); | ||
|
||
prebuilt_path | ||
.exists() | ||
.then_some(prebuilt_path) | ||
.ok_or_else(|| anyhow!("prebuilt library not found")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the path is expected to not be defined in some cases, this should be an |
||
} | ||
|
||
fn is_prebuilt(&self) -> bool { | ||
self.prebuilt_lib_path().is_ok() | ||
} | ||
Comment on lines
+95
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I find this function more confusing than helpful, as all it does is check if path exist - it still doesn't mean that macro can be actually loaded, or that the compilation will not be run, but the name does not reflect that. Maybe we should just inline |
||
} | ||
|
||
pub fn compile_unit(unit: ProcMacroCompilationUnit, ws: &Workspace<'_>) -> Result<()> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the same prefix for all this tests, and use the prefix to filter here, so we only run Cargo once.