Load prebuilt macros #1812

DelevoXDG · 2024-12-05T17:57:13Z

…rebuilts

maciektr · 2024-12-06T09:20:45Z

scarb/src/compiler/compilation_unit.rs

@@ -130,6 +131,8 @@ pub trait CompilationUnitAttributes {
    fn components(&self) -> &[CompilationUnitComponent];
    fn digest(&self) -> String;

+    fn is_prebuilt(&self) -> bool;


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.

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

The trait CompilationUnitAttributes is shared between CompilationUnit, CairoCompilationUnit and ProcMacroCompilationUnit. Shouldn't this be defined only for the last one? I don't think it's used on the former two anyway.

maciektr · 2024-12-06T09:23:17Z

scarb/src/ops/compile.rs

 }

 #[tracing::instrument(skip_all, level = "debug")]
 fn process<F>(
    packages: Vec<PackageId>,
    opts: CompileOpts,
    ws: &Workspace<'_>,
+    skip_prebuilt_proc_macros: bool,


Why do we need this?

We want to run compile_unit with this flag:

True by default - prebuilts should be skipped initially

If loading prebuilts fails, we want to be able to call it without skipping prebuilts

Since process takes mut operation: F including compile_unit and later runs it, process<F> had to be updated as well.

Since we currently always run process with skip_prebuilt_proc_macros = true, I've considered not adding as flag, and just having:

operation(compilation_units, ws, true)?;

But, that just seemed less readable to me.

maciektr · 2024-12-06T09:44:16Z

scarb/src/compiler/plugin/proc_macro/compilation.rs

+            name = self.id.name,
+            version = self.id.version,
+            target = target_triple,
+            suffix = DLL_SUFFIX


Can we use libloading::library_filename instead, to be consistent with shared_lib_path?

I've considered this as well. Unfortunately, it will lead to naming conflicts, as for example library with a name libsome_macro.so will be produced both for aarch64-unknown-linux-gnu and x86_64-unknown-linux-gnu.

Therefore I used format as described in the issue:

Within this directory, binaries for all platforms will follow the following naming pattern: ${PACKAGE_NAME}_v${PACKAGE_VERSION}_${TARGET}.${so|dylib|dll}

On the second look though

some_macro_v0.1.0_aarch64-apple-darwin.dylib some_macro_v0.1.0_aarch64-unknown-linux-gnu.so some_macro_v0.1.0_x86_64-apple-darwin.dylib some_macro_v0.1.0_x86_64-unknown-linux-gnu.so

we may consider dropping ${PACKAGE_NAME}_v${PACKAGE_VERSION}_ to slightly simplify these.

I just meant replace DLL_SUFFIX with call to libloading::library_filename which adds the extension for you

libloading::library_filename

It also adds DLL_PREFIX, which can be either "" or lib, which seems more confusing than helpful, especially since users need to name the files manually.

What do you think about dropping the ${PACKAGE_NAME}_v${PACKAGE_VERSION}_ prefix though? It would simplify things and prevent naming issues caused by mismatched Cargo and Scarb names or versions. The downside is that users might accidentally reuse a prebuilt binary after a version update, but this seems like a minor issue to me.

It also adds DLL_PREFIX, which can be either "" or lib, which seems more confusing than helpful, especially since users need to name the files manually.

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.

naming issues caused by mismatched Cargo and Scarb names or versions

What do you mean?

What do you mean?

I’m referring to the reason behind this warning.

When naming build artifacts, Cargo uses the package name specified in the Cargo manifest, which:

May differ from the name specified in the Scarb manifest: Support packaging cairo-plugins #1605 (comment)

Must be resolved manually via cargo_metadata: Support packaging cairo-plugins #1605 (comment)

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.
The problem with 2. is that using proc macros without Cargo is one of the motivations for introducing loading prebuilts. Relying on cargo_metadata would make that impossible.

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.

While it’s true that Cargo compiles libraries with a prefix, the resulting libraries must be renamed anyway to avoid conflicts:

libsome_macro.dylib libsome_macro.so libsome_macro.dylib libsome_macro.so some_macro.dll

Since renaming is inevitable, I argue that the simplest and most consistent naming convention - requiring minimal amount of documentation reading from the user - is:

aarch64-apple-darwin.dylib aarch64-unknown-linux-gnu.so x86_64-apple-darwin.dylib x86_64-unknown-linux-gnu.so x86_64-pc-windows-msvc.so

This approach seems preferable to me alternatives like:

libsome_macro_v0.1.0_aarch64-apple-darwin.dylib libsome_macro_v0.1.0_aarch64-unknown-linux-gnu.so libsome_macro_v0.1.0_x86_64-apple-darwin.dylib libsome_macro_v0.1.0_x86_64-unknown-linux-gnu.so some_macro_v0.1.0_x86_64-pc-windows-msvc.so

or:

libaarch64-apple-darwin.dylib libaarch64-unknown-linux-gnu.so libx86_64-apple-darwin.dylib libx86_64-unknown-linux-gnu.so x86_64-pc-windows-msvc.dll

What do you think?

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.

True! Sounds like we should turn this warning into a hard error soon, to warn macro authors early on about the mismatch 😄

While it’s true that Cargo compiles libraries with a prefix, the resulting libraries must be renamed anyway to avoid conflicts:

Makes sense. So let's drop the prefix indeed. I would still keep the name + version, just to make sure.

So let's drop the prefix indeed.

👍🏻

I would still keep the name + version, just to make sure.

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.

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.

scarb/tests/build_cairo_plugin.rs

maciektr · 2024-12-06T10:02:26Z

scarb/src/compiler/plugin/proc_macro/host.rs

@@ -1156,6 +1156,13 @@ impl ProcMacroHost {
        Ok(())
    }

+    /// Registers a prebuilt procedural macro by loading the shared library from the specified path.
+    pub fn register_prebuilt(&mut self, package: Package, config: &Config) -> Result<()> {


I don't see why this needs to be a separate function. Can't ProcMacroHost::register decide which logic should be used?

I assume what you mean is ProcMacroHost::register should be making the decision on loading mechanism implicitly based on the package being in the allow-list and prebuilt binary being present.

When loading procmacro package ~~that is present in the allowlist~~ (#1769), Scarb will look for the /target/scarb/cairo-plugin directory and will attempt to load a matching binary if present. In any case this loading process fails, Scarb will always fall back to source-code compilation. This will happen silently without any warnings.

However, we want to be able to attempt loading from prebuilts, and if that resulted in failure for any reason, explicitly compile and load the binaries instead.

scarb/src/compiler/plugin/proc_macro/ffi.rs

maciektr · 2024-12-06T10:32:39Z

scarb/src/compiler/db.rs

+                ws.config()
+                    .ui()
+                    .print(Status::new("Compiling", &plugin_unit.name()));
+                compile_unit(plugin_unit, ws, false)?;


In case of corrupted prebuilt file, this would cause the macro compilation to be run on each compilation unit that depends on the plugin. For instance in workspace of 5 packages, each with lib and starknet-contract targets, compilation of snforge plugin that failed to load as pre-built would be called 10 times (and print statuses to ui).
Maybe instead we can return a typed error from compile_unit_inner function, e.g. RetryWithPrerequisite ( Vec::<_>) that would list additional compilation units that need to be run before this one?
Then we can change compile_units from for unit in units to a FILO stack of CUs to compile, that in case of RetryWithPrerequisite would add the failed CU back to FILO with the returned prerequisites and try again.

Maybe instead we can return a typed error from compile_unit_inner function, e.g. RetryWithPrerequisite ( Vec::<_>) that would list additional compilation units that need to be run before this one?

compile_unit_inner does not load proc macros though.

I think the simplier approach would be to update load_plugins to just skip compilation if shared_lib_path already exists for the plugin. What do you think?

Ok, you are right.
But I still don't like calling compile_unit in load_plugins or any other place that is not compile_units 😛 It would make the execution flow a bit confusing, both from the code perspective and from the user one. For instance, we could end up printing "Compiling A", then print "Compiling B", then compile B and only then actually start compiling A. And what if we decided we need to compile some comp. units in parallel in future?

skip compilation if shared_lib_path already exists for the plugin

This is not necessarily true. For instance, when you depend on a local proc macro by path, it will always have the same target dir location in cache (name-path/version/), but it will require recompilation with Cargo on any changes made to it.

I have another idea:

Maybe we should add an optional field with type Option<Arc<ProcMacroInstance>> and name like prebuilt or smth to ProcMacroCompilationUnit and CompilationUnitCairoPlugin? We could then load prebuilts in generate_compilation_units. If loading a prebuilt fails, we just set this field to None. The compile_unit could compile if the field is None, and just do nothing otherwise. Since it is an Arc, we would only load it once and then pass everywhere where needed. The load_plugins function can just use this field if present directly as it just needs the Arc anyway. This field would not be exposed in metadata, and we could add skip_loading_prebuilts to generate_compilation_units for the metadata command.

WDYT about this?

Signed-off-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

…ifest

maciektr

Great effort! 💪

maciektr · 2024-12-19T17:40:45Z

.github/workflows/ci.yml

+          cargo test -p scarb --test build_cairo_plugin compile_with_prebuilt_plugins -- --exact
+          cargo test -p scarb --test build_cairo_plugin compile_with_invalid_prebuilt_plugins -- --exact
+          cargo test -p scarb --test proc_macro_server load_prebuilt_proc_macros -- --exact


Let's use the same prefix for all this tests, and use the prefix to filter here, so we only run Cargo once.

maciektr · 2024-12-19T17:45:26Z

scarb/src/compiler/plugin/proc_macro/compilation.rs

+            name = self.id.name,
+            version = self.id.version,
+            target = target_triple,
+            suffix = DLL_SUFFIX


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.

maciektr · 2024-12-19T17:53:43Z

scarb/src/ops/resolve.rs

    ignore_cairo_version: bool,
+    load_prebuilts: bool,


Can we create a struct, e.g. CompilationUnitsOpts to name those? Passing raw boolean flag can be confusing at times.

maciektr · 2024-12-19T17:55:53Z

scarb/src/compiler/compilation_unit.rs

@@ -130,6 +131,8 @@ pub trait CompilationUnitAttributes {
    fn components(&self) -> &[CompilationUnitComponent];
    fn digest(&self) -> String;

+    fn is_prebuilt(&self) -> bool;


The trait CompilationUnitAttributes is shared between CompilationUnit, CairoCompilationUnit and ProcMacroCompilationUnit. Shouldn't this be defined only for the last one? I don't think it's used on the former two anyway.

maciektr · 2024-12-19T17:58:10Z

scarb/src/compiler/compilation_unit.rs

+    fn is_prebuilt(&self) -> bool {
+        self.components
+            .first()
+            .map(|c| c.package.is_prebuilt())
+            .unwrap_or(false)
+    }


Shouldn't this be just self.prebuilt.is_some()?

maciektr · 2024-12-19T18:01:23Z

scarb/src/compiler/plugin/proc_macro/compilation.rs

+        prebuilt_path
+            .exists()
+            .then_some(prebuilt_path)
+            .ok_or_else(|| anyhow!("prebuilt library not found"))


If the path is expected to not be defined in some cases, this should be an Option instead of Result. Let's handle the error where path missing is an actual error.

maciektr · 2024-12-19T18:02:58Z

scarb/src/compiler/plugin/proc_macro/compilation.rs

+    fn is_prebuilt(&self) -> bool {
+        self.prebuilt_lib_path().is_ok()
+    }


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 prebuilt_path().is_some() instead? Sounds short enough to me. 🤔

maciektr · 2024-12-19T18:15:15Z

scarb/src/ops/resolve.rs

@@ -619,9 +626,13 @@ impl<'a> PackageSolutionCollector<'a> {
                // We can safely unwrap as all packages with `PackageClass::CairoPlugin` must define plugin target.
                let target = package.target(&TargetKind::CAIRO_PLUGIN).unwrap();
                let props: CairoPluginProps = target.props()?;
+                let prebuilt = ProcMacroInstance::try_load_prebuilt(package.clone())


I am not sure, if loading the same dynamic library into the memory multiple times is legal (especially on windows). For safety, I would do it in two steps

First generate all CairoCompilationUnits

Then iter through all cairo_plugins and create ProceduralMacroCompilationUnits for them (loading the plugins)

And then, reiterate through CairoCompilationUnits, cloning the Arc with loaded macro into each maching CairoCompilationUnitComponent.

This way we do not need to load any macro twice.
Also, it might be worth the effort, to replicate this situation in tests - i.e. instead of a single package, have two packages relying on the same plugin in a single workspace.

maciektr · 2024-12-19T18:20:20Z

scarb/src/compiler/plugin/mod.rs

-    // 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() {


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.

maciektr · 2024-12-20T12:44:35Z

Replaced with #1856

DelevoXDG added 9 commits December 5, 2024 17:57

Update SharedLibraryProvider to resolve prebuilt path

b35f947

Add helper to determine whether the package is prebuilt

908cc47

Update ProcMacroInstance and ProcMacroInstance to allow loading p…

7d399d3

…rebuilts

Update load_plugins to load prebuilts

a95a25d

Add skip_prebuilt_proc_macros flag and relevant logic

2aa6ee8

Update load_plugins to print compilation message to ui

bfeb684

Update proc_macro_server

e570a99

Add simple test

c73c0f6

misc

87f87c5

maciektr reviewed Dec 6, 2024

View reviewed changes

DelevoXDG and others added 3 commits December 6, 2024 12:36

fix test

a4a8d14

Signed-off-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

Merge try_new and try_load_prebuilt

c81d9ce

Fix test

9cac622

DelevoXDG requested a review from maciektr December 6, 2024 12:40

DelevoXDG added 14 commits December 6, 2024 18:42

Partially fix name resolution

eba6fe4

Add system-universal test

57cc3d1

Update load_plugins to try registering before compiling

6b342b1

Add load_plugins comments

e76fcf2

Fix false positive is_prebuilt

8424cb6

Fix false positive is_prebuilt

be7b5c2

misc

1e2845a

misc

8a468d3

Use package name and version from Scarb manifest instead of Cargo man…

394bf55

…ifest

misc: unused arg

7b0162c

Ensure cargo fetch is not called for prebuilt plugins

76118b0

Update test

05cf39d

Add test with invalid prebuilt plugins

5a85ee2

Fix comment

cce42b7

DelevoXDG marked this pull request as ready for review December 8, 2024 22:51

DelevoXDG requested a review from Draggu as a code owner December 8, 2024 22:51

DelevoXDG requested a review from a team as a code owner December 8, 2024 22:51

DelevoXDG added 3 commits December 9, 2024 00:17

Format

06aab97

Add test-prebuilt-plugins for all available targets

64f362e

Add proc-macro-server test

e1873fd

DelevoXDG force-pushed the zdobnikau/load-prebuilt-macros branch from 981d3df to e1873fd Compare December 11, 2024 10:10

DelevoXDG added 4 commits December 16, 2024 20:50

Preload prebuilts during compilation

034af9a

Adjust proc macro server to loading prebuilts on compilation

b8c4156

Format

3af99af

Reorder load_prebuilts in generate_compilation_units

9eb6fc4

maciektr reviewed Dec 19, 2024

View reviewed changes

maciektr closed this Dec 20, 2024

maciektr mentioned this pull request Dec 20, 2024

Load prebuilt macros #1856

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load prebuilt macros #1812

Load prebuilt macros #1812

DelevoXDG commented Dec 5, 2024

maciektr Dec 6, 2024

DelevoXDG Dec 6, 2024 •

edited

Loading

maciektr Dec 19, 2024

maciektr Dec 6, 2024

DelevoXDG Dec 6, 2024 •

edited

Loading

maciektr Dec 6, 2024

DelevoXDG Dec 6, 2024

maciektr Dec 6, 2024

DelevoXDG Dec 8, 2024 •

edited

Loading

maciektr Dec 13, 2024

DelevoXDG Dec 16, 2024

maciektr Dec 16, 2024

DelevoXDG Dec 18, 2024

maciektr Dec 19, 2024

maciektr Dec 6, 2024

DelevoXDG Dec 6, 2024

maciektr Dec 6, 2024

DelevoXDG Dec 6, 2024

maciektr Dec 13, 2024

maciektr left a comment •

edited

Loading

maciektr Dec 19, 2024

maciektr Dec 19, 2024

maciektr Dec 19, 2024

maciektr Dec 19, 2024

maciektr Dec 19, 2024

maciektr Dec 19, 2024

maciektr Dec 19, 2024

maciektr Dec 19, 2024

maciektr Dec 19, 2024

maciektr commented Dec 20, 2024

Load prebuilt macros #1812

Load prebuilt macros #1812

Conversation

DelevoXDG commented Dec 5, 2024

Choose a reason for hiding this comment

DelevoXDG Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DelevoXDG Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DelevoXDG Dec 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciektr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciektr commented Dec 20, 2024

DelevoXDG Dec 6, 2024 •

edited

Loading

DelevoXDG Dec 6, 2024 •

edited

Loading

DelevoXDG Dec 8, 2024 •

edited

Loading

maciektr left a comment •

edited

Loading