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

Aot macro compilation - magic script #1789

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions scarb/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ windows-sys.workspace = true
zstd.workspace = true
cargo_metadata.workspace = true
flate2.workspace = true
fs_extra.workspace = true

[target.'cfg(not(target_os = "linux"))'.dependencies]
reqwest = { workspace = true, default-features = true }
Expand Down
1 change: 1 addition & 0 deletions scarb/src/core/manifest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ pub struct ManifestMetadata {
pub license_file: Option<Utf8PathBuf>,
pub readme: Option<Utf8PathBuf>,
pub repository: Option<String>,
pub include: Option<Vec<String>>,
Copy link
Member

Choose a reason for hiding this comment

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

this field should be a separate PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I just thought that adding this single field isn't that much of a big change to make it as a separate PR, but I see your point, should i convert this to stacked PR?

Copy link
Member

Choose a reason for hiding this comment

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

I think so

#[serde(rename = "tool")]
pub tool_metadata: Option<BTreeMap<SmolStr, Value>>,
pub cairo_version: Option<VersionReq>,
Expand Down
8 changes: 8 additions & 0 deletions scarb/src/core/manifest/toml_manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ pub struct PackageInheritableFields {
pub license_file: Option<Utf8PathBuf>,
pub readme: Option<PathOrBool>,
pub repository: Option<String>,
pub include: Option<Vec<String>>,
pub cairo_version: Option<VersionReq>,
}

Expand Down Expand Up @@ -147,6 +148,7 @@ impl PackageInheritableFields {
get_field!(license, String);
get_field!(license_file, Utf8PathBuf);
get_field!(repository, String);
get_field!(include, VecOfStrings);
get_field!(edition, Edition);

pub fn readme(&self, workspace_root: &Utf8Path, package_root: &Utf8Path) -> Result<PathOrBool> {
Expand Down Expand Up @@ -197,6 +199,7 @@ pub struct TomlPackage {
pub license_file: Option<MaybeWorkspaceField<Utf8PathBuf>>,
pub readme: Option<MaybeWorkspaceField<PathOrBool>>,
pub repository: Option<MaybeWorkspaceField<String>>,
pub include: Option<MaybeWorkspaceField<Vec<String>>>,
/// **UNSTABLE** This package does not depend on Cairo's `core`.
pub no_core: Option<bool>,
pub cairo_version: Option<MaybeWorkspaceField<VersionReq>>,
Expand Down Expand Up @@ -571,6 +574,11 @@ impl TomlManifest {
.clone()
.map(|mw| mw.resolve("repository", || inheritable_package.repository()))
.transpose()?,
include: package
.include
.clone()
.map(|mw| mw.resolve("include", || inheritable_package.include()))
.transpose()?,
cairo_version: package
.cairo_version
.clone()
Expand Down
1 change: 1 addition & 0 deletions scarb/src/core/publishing/manifest_normalization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ fn generate_package(pkg: &Package) -> Box<TomlPackage> {
.clone()
.map(|_| MaybeWorkspace::Defined((Utf8PathBuf::from(DEFAULT_README_FILE_NAME)).into())),
repository: metadata.repository.clone().map(MaybeWorkspace::Defined),
include: metadata.include.clone().map(MaybeWorkspace::Defined),
no_core: summary.no_core.then_some(true),
cairo_version: metadata.cairo_version.clone().map(MaybeWorkspace::Defined),
experimental_features: pkg.manifest.experimental_features.clone(),
Expand Down
2 changes: 2 additions & 0 deletions scarb/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,5 @@ pub const TEST_ASSERTS_PLUGIN_NAME: &str = "assert_macros";
pub const CAIRO_RUN_PLUGIN_NAME: &str = "cairo_run";
pub const CARGO_MANIFEST_FILE_NAME: &str = "Cargo.toml";
pub const CARGO_LOCK_FILE_NAME: &str = "Cargo.lock";
pub static SHARED_LIBRARY_TARGET_DIRECTORY: LazyLock<Utf8PathBuf> =
maciektr marked this conversation as resolved.
Show resolved Hide resolved
LazyLock::new(|| ["target", "scarb", "cairo-plugin"].iter().collect());
97 changes: 89 additions & 8 deletions scarb/src/ops/package.rs
wawel37 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
use anyhow::{anyhow, bail, ensure, Context, Result};
use camino::{Utf8Path, Utf8PathBuf};
use core::str;
use fs::copy;
use fs_extra::dir::{copy as fs_extra_copy, CopyOptions};
use fs_extra::error::{Error as FsExtraError, ErrorKind as FsExtraErrorKind};
use indoc::{formatdoc, indoc, writedoc};
use std::collections::BTreeMap;
use std::fs::File;
use std::fs::{self, File};
use std::io::{Seek, SeekFrom, Write};

use anyhow::{bail, ensure, Context, Result};
use camino::Utf8PathBuf;
use indoc::{formatdoc, indoc, writedoc};
use std::path::Path;

use crate::core::registry::package_source_store::PackageSourceStore;
use crate::sources::client::PackageRepository;
Expand All @@ -23,9 +27,11 @@ use crate::flock::{FileLockGuard, Filesystem};
use crate::internal::restricted_names;
use crate::{
ops, CARGO_MANIFEST_FILE_NAME, DEFAULT_LICENSE_FILE_NAME, DEFAULT_README_FILE_NAME,
MANIFEST_FILE_NAME, VCS_INFO_FILE_NAME,
MANIFEST_FILE_NAME, SHARED_LIBRARY_TARGET_DIRECTORY, VCS_INFO_FILE_NAME,
};

use super::execute_script;

const VERSION: u8 = 1;
const VERSION_FILE_NAME: &str = "VERSION";
const ORIGINAL_MANIFEST_FILE_NAME: &str = "Scarb.orig.toml";
Expand Down Expand Up @@ -157,6 +163,7 @@ fn package_one_impl(
ws: &Workspace<'_>,
) -> Result<FileLockGuard> {
let pkg = ws.fetch_package(&pkg_id)?;
let target_dir = ws.target_dir().child("package");

ws.config()
.ui()
Expand All @@ -166,14 +173,44 @@ fn package_one_impl(
check_metadata(pkg, ws.config())?;
}

let recipe = prepare_archive_recipe(pkg, opts, ws)?;
let mut recipe: Vec<ArchiveFile> = Vec::default();

if let Some(script_definition) = pkg.manifest.scripts.get("package") {
let target_dir_path = target_dir.path_existent()?;
ws.config().ui().print(Status::new(
"Running package script with package",
&pkg_id.to_string(),
));

if let Some(includes) = pkg.manifest.metadata.include.clone() {
copy_items_to_target_dir(includes, target_dir_path)?;
}

execute_script(script_definition, &[], ws, target_dir_path, None)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we executing the script in target/package?

It seems to me it should be either executed in the root dir, or if we want avoid modifying any unnecessary files there, in target/package/<name>-<version>?


let built_macros_target_dir = ws.target_dir().child("scarb").child("cairo-plugin");

if built_macros_target_dir.exists() {
let dynamic_library_files =
find_dynamic_library_files(built_macros_target_dir.path_unchecked());
for dynamic_library_file in dynamic_library_files.into_iter() {
recipe.push(ArchiveFile {
path: SHARED_LIBRARY_TARGET_DIRECTORY
.as_path()
.join(dynamic_library_file.file_name().unwrap()),
contents: ArchiveFileContents::OnDisk(dynamic_library_file),
})
}
}
}

recipe.extend(prepare_archive_recipe(pkg, opts, ws)?);
let num_files = recipe.len();

// Package up and test a temporary tarball and only move it to the final location if it actually
// passes all verification checks. Any previously existing tarball can be assumed as corrupt
// or invalid, so we can overwrite it if it exists.
let filename = pkg_id.tarball_name();
let target_dir = ws.target_dir().child("package");

let mut dst =
target_dir.create_rw(format!(".{filename}"), "package scratch space", ws.config())?;
Expand Down Expand Up @@ -606,3 +643,47 @@ fn check_metadata(pkg: &Package, config: &Config) -> Result<()> {

Ok(())
}

fn copy_items_to_target_dir(paths: Vec<String>, target_dir: &Utf8Path) -> Result<()> {
let target_path = Path::new(target_dir);
let options = CopyOptions::new().copy_inside(true).overwrite(true);

for path_str in paths {
let source_path = Path::new(&path_str);
if source_path.exists() {
if source_path.is_dir() {
fs_extra_copy(source_path, target_path, &options)?;
} else {
let file_name = source_path.file_name().ok_or(FsExtraError::new(
fs_extra::error::ErrorKind::Other,
"failed to extract file name",
))?;
let target_file_path = target_path.join(file_name);
copy(source_path, target_file_path).map_err(|err| {
FsExtraError::new(FsExtraErrorKind::Io(err), "failed to copy file")
})?;
}
} else {
return Err(anyhow!("path does not exist: {}", source_path.display()));
}
}

Ok(())
}

fn find_dynamic_library_files(directory: &Utf8Path) -> Vec<Utf8PathBuf> {
let mut files = Vec::new();
if let Ok(entries) = fs::read_dir(directory) {
for entry in entries.filter_map(Result::ok) {
let path = entry.path();
if let Ok(utf8_path) = Utf8PathBuf::from_path_buf(path) {
if let Some(extension) = utf8_path.extension() {
if extension == "dll" || extension == "so" || extension == "dylib" {
files.push(utf8_path);
}
}
}
}
}
files
}
111 changes: 107 additions & 4 deletions scarb/tests/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,11 @@ impl PackageChecker {
.path()
.expect("failed to get archive entry path")
.into_owned();
let mut contents = String::new();
entry
.read_to_string(&mut contents)
.expect("failed to read archive entry contents");
let mut contents = String::default();
// Some files are not valid utf-8 contents, so we don't want to read them at all.
// We just want to track that they even exist.
let _ = entry.read_to_string(&mut contents);

(name, contents)
})
.collect();
Expand Down Expand Up @@ -1525,3 +1526,105 @@ fn package_with_publish_disabled() {
[..]Packaged [..] files, [..] ([..] compressed)
"#});
}

#[test]
fn package_with_package_script() {
let t = TempDir::new().unwrap();

#[cfg(not(windows))]
let script_code = indoc! { r#"cargo build --release && mkdir -p ../scarb/cairo-plugin && cp target/release/libfoo.so ../scarb/cairo-plugin"#};

#[cfg(windows)]
let script_code = indoc! { r#"cargo build --release && mkdir -p ../scarb/cairo-plugin && cp target/release/libfoo.dll ../scarb/cairo-plugin"#};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the users are not supposed to use relatives paths like ../scarb/cairo-plugin or ../scarb/cairo-plugin, and binaries should not be copied somewhere outside of the cwd tree.

The binaries imo should either:

  1. Be required to be placed in target/scarb/cairo-plugin/
  2. Be automatically located regardless of their path and then placed in the correct directory


CairoPluginProjectBuilder::start()
.name("foo")
.scarb_project(|b| {
b.name("foo")
.version("1.0.0")
.manifest_package_extra(formatdoc! {r#"
include = ["Cargo.lock", "Cargo.toml", "src"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's supposed to work that way 🤔

These files are included in the tarball by default, shouldn't we use all the files that are included in the tarball by default when running a package script?

"#})
.manifest_extra(formatdoc! {r#"
[cairo-plugin]

[scripts]
package = "{script_code}"
"#})
})
.lib_rs(indoc! {r#"
use cairo_lang_macro::{ProcMacroResult, TokenStream, attribute_macro};

#[attribute_macro]
pub fn some(_attr: TokenStream, token_stream: TokenStream) -> ProcMacroResult {
ProcMacroResult::new(token_stream)
}
"#})
.build(&t);

Scarb::quick_snapbox()
.current_dir(&t)
.arg("package")
.assert()
.success();

#[cfg(not(windows))]
let expected_shared_lib_file = r#"target/scarb/cairo-plugin/libfoo.so"#;

#[cfg(windows)]
let expected_shared_lib_file = r#"target/scarb/cairo-plugin/libfoo.dll"#;

PackageChecker::assert(&t.child("target/package/foo-1.0.0.tar.zst"))
.name_and_version("foo", "1.0.0")
.contents(&[
"VERSION",
"Scarb.orig.toml",
"Scarb.toml",
expected_shared_lib_file,
"Cargo.toml",
"Cargo.orig.toml",
"src/lib.rs",
]);
}

#[test]
fn package_with_package_script_not_existing_script() {
let t = TempDir::new().unwrap();

#[cfg(not(windows))]
let script_name = "script.sh";

#[cfg(windows)]
let script_name = "script.bat";

CairoPluginProjectBuilder::start()
.name("foo")
.scarb_project(|b| {
b.name("foo")
.version("1.0.0")
.manifest_package_extra(indoc! {r#"
include = ["Cargo.lock", "Cargo.toml", "src", "script.sh"]
"#})
.manifest_extra(formatdoc! {r#"
[cairo-plugin]

[scripts]
package = "{script_name}"
"#})
})
.lib_rs(indoc! {r#"
use cairo_lang_macro::{ProcMacroResult, TokenStream, attribute_macro};

#[attribute_macro]
pub fn some(_attr: TokenStream, token_stream: TokenStream) -> ProcMacroResult {
ProcMacroResult::new(token_stream)
}
"#})
.build(&t);

Scarb::quick_snapbox()
.current_dir(&t)
.arg("package")
.assert()
.failure();
}
Loading