-
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
Aot macro compilation - magic script #1789
Changes from all commits
205989d
77dcb33
89299fa
e6dc3a9
aa1ca15
61cca2e
c28fa31
4a68d19
ef0450f
b0fdf32
9bd5a91
98f6064
298e8a9
ab95831
4009be9
fb66fcf
6a66b47
81dcccc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
|
@@ -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"; | ||
|
@@ -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() | ||
|
@@ -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)?; | ||
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. Why are we executing the script in It seems to me it should be either executed in the root dir, or if we want avoid modifying any unnecessary files there, in |
||
|
||
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())?; | ||
|
@@ -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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
|
@@ -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"#}; | ||
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 think the users are not supposed to use relatives paths like The binaries imo should either:
|
||
|
||
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"] | ||
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 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(); | ||
} |
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.
this field should be a separate PR
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.
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?
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.
I think so