Skip to content

Commit

Permalink
Bug fix; re-run const resolution on change (#1053)
Browse files Browse the repository at this point in the history
* Bug fix; re-run const resolution on change

* Fix Da Address type (#1054)

* address code review feedback

* set constants manifest for trybuild

---------

Co-authored-by: Blazej Kolad <blazejkolad@gmail.com>
  • Loading branch information
preston-evans98 and bkolad authored Oct 17, 2023
1 parent 27ae049 commit 606d7b8
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 72 deletions.
4 changes: 4 additions & 0 deletions module-system/sov-modules-macros/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ schemars = { workspace = true }
serde_json = { workspace = true }
syn = { version = "1.0", features = ["full"] }


[build-dependencies]
anyhow = { workspace = true }

[features]
default = []
native = ["jsonrpsee"]
66 changes: 60 additions & 6 deletions module-system/sov-modules-macros/build.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,68 @@
use std::path::PathBuf;
use std::path::{Path, PathBuf};
use std::{env, fs, io};

fn main() -> io::Result<()> {
use anyhow::{anyhow, Context};

fn resolve_manifest_path(out_dir: &Path) -> anyhow::Result<PathBuf> {
let manifest = "constants.json";
match env::var("CONSTANTS_MANIFEST"){
Ok(p) if p.is_empty() => {
Err(anyhow!(
"failed to read target path for sovereign manifest file: env var `CONSTANTS_MANIFEST` was set to the empty string"
))
},
Ok(p) => PathBuf::from(&p).canonicalize().map_err(|e| {
anyhow!("failed to canonicalize path for sovereign manifest file from env var `{p}`: {e}")
}),
Err(_) => {
// Start from the parent of out-dir to avoid a self-referential `cp` once the output file has been placed into `out_dir` for the first time
let current_path = out_dir.parent().ok_or(anyhow!("out dir must have parent but {out_dir:?} had none"))?;
let mut current_path = current_path.canonicalize().with_context(|| anyhow!("failed to canonicalize path to otput dirdir `{current_path:?}`"))?;
loop {
let path = current_path.join(manifest);
if path.exists() && path.is_file() {
return Ok(path);
}

current_path = current_path.parent().ok_or_else(|| {
anyhow!("Could not find a parent `{manifest}`")
})?.to_path_buf();
}
}
}
}

fn main() -> anyhow::Result<()> {
// writes the target output dir into the manifest path so it can be later read for the
// resolution of the sovereign.toml manifest file
let target = env::var("OUT_DIR").map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
let target = PathBuf::from(target).canonicalize()?.display().to_string();
let out_dir = env::var("OUT_DIR").map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
let out_dir = PathBuf::from(out_dir);
let out_dir = out_dir
.canonicalize()
.with_context(|| anyhow!("could not canonicalize out dir `{out_dir:?}`"))?;
let manifest = env!("CARGO_MANIFEST_DIR");
let manifest = PathBuf::from(manifest).canonicalize()?.join("target-path");
fs::write(manifest, target)?;
let target_path_record = PathBuf::from(manifest)
.canonicalize()
.with_context(|| anyhow!("could not canonicalize manifest dir path `{manifest:?}`"))?
.join("target-path");

// Write the path "OUT_DIR" to a file in the manifest directory so that it's available to macros
fs::write(target_path_record, out_dir.display().to_string())?;

let input_manifest_path = resolve_manifest_path(&out_dir)?;
let output_manifest_path = out_dir.join("constants.json");

// Copy the constants.json file into out_dir
let bytes_copied = fs::copy(&input_manifest_path, &output_manifest_path).with_context(|| anyhow!("could not copy manifest from {manifest:?} to output directory `{output_manifest_path:?}`"))?;
if bytes_copied == 0 {
return Err(anyhow!("Could not find valid `constants.json` Manifest file. The file at `{input_manifest_path:?}` was empty. You can set a different input file with the `CONSTANTS_MANIFEST` env var"));
}
// Tell cargo to rebuild if constants.json changes
// We need to watch the output file in to handle the corner case where the user updates
// their input manifest from `a/constants.json` to a pre-existing `b/constants.json` without updating any rust files.
// Othewise, Cargo's timestamp based change detection will miss these changes.
println!("cargo:rerun-if-changed={}", output_manifest_path.display());
println!("cargo:rerun-if-changed={}", input_manifest_path.display());

Ok(())
}
86 changes: 25 additions & 61 deletions module-system/sov-modules-macros/src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,76 +48,40 @@ impl<'a> Manifest<'a> {
///
/// The `parent` is used to report the errors to the correct span location.
pub fn read_constants(parent: &'a Ident) -> Result<Self, syn::Error> {
let manifest = "constants.json";
let initial_path = match env::var("CONSTANTS_MANIFEST"){
Ok(p) if p.is_empty() => {
Err(Self::err(
&p,
parent,
"failed to read target path for sovereign manifest file: env var `CONSTANTS_MANIFEST` was set to the empty string".to_string(),
))
},
Ok(p) => PathBuf::from(&p).canonicalize().map_err(|e| {
let target_path_pointer = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.canonicalize()
.map_err(|e| {
Self::err(
&p,
"target-path",
parent,
format!("failed to canonicalize path for sovereign manifest file from env var `{p}`: {e}"),
format!("failed access base dir for sovereign manifest file: {e}"),
)
}),
Err(_) => {
// read the target directory set via build script since `OUT_DIR` is available only at build
let initial_path = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.canonicalize()
.map_err(|e| {
Self::err(
manifest,
parent,
format!("failed access base dir for sovereign manifest file: {e}"),
)
})?
.join("target-path");

let initial_path = fs::read_to_string(&initial_path).map_err(|e| {
Self::err(
&initial_path,
parent,
format!("failed to read target path for sovereign manifest file: {e}"),
)
})?;

PathBuf::from(initial_path.trim())
.canonicalize()
.map_err(|e| {
Self::err(
&initial_path,
parent,
format!("failed access base dir for sovereign manifest file: {e}"),
)
})
}
}?;

let path: PathBuf;
let mut current_path = initial_path.as_path();
loop {
if current_path.join(manifest).exists() {
path = current_path.join(manifest);
break;
}
})?
.join("target-path");

let manifest_path = fs::read_to_string(&target_path_pointer).map_err(|e| {
Self::err(
&target_path_pointer,
parent,
format!("failed to read target path for sovereign manifest file: {e}"),
)
})?;

current_path = current_path.parent().ok_or_else(|| {
let manifest_path = PathBuf::from(manifest_path.trim())
.canonicalize()
.map_err(|e| {
Self::err(
current_path,
&manifest_path,
parent,
format!("Could not find a parent `{manifest}`"),
format!("failed access base dir for sovereign manifest file: {e}"),
)
})?;
}
})?
.join("constants.json");

let manifest = fs::read_to_string(&path)
.map_err(|e| Self::err(current_path, parent, format!("failed to read file: {e}")))?;
let manifest = fs::read_to_string(&manifest_path)
.map_err(|e| Self::err(&manifest_path, parent, format!("failed to read file: {e}")))?;

Self::read_str(manifest, path, parent)
Self::read_str(manifest, manifest_path, parent)
}

/// Gets the requested object from the manifest by key
Expand Down
22 changes: 17 additions & 5 deletions module-system/sov-modules-macros/tests/all_tests.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,18 @@
use std::path::PathBuf;

fn set_constants_manifest() {
let manifest_dir = std::env::var_os("CARGO_MANIFEST_DIR").unwrap();
std::env::set_var(
"CONSTANTS_MANIFEST",
PathBuf::from(manifest_dir)
.join("tests")
.join("constants.json"),
);
}

#[test]
fn module_info_tests() {
set_constants_manifest();
let t = trybuild::TestCases::new();
t.pass("tests/module_info/parse.rs");
t.pass("tests/module_info/mod_and_state.rs");
Expand All @@ -19,6 +30,7 @@ fn module_info_tests() {

#[test]
fn module_dispatch_tests() {
set_constants_manifest();
let t = trybuild::TestCases::new();
t.pass("tests/dispatch/derive_genesis.rs");
t.pass("tests/dispatch/derive_dispatch.rs");
Expand All @@ -27,6 +39,7 @@ fn module_dispatch_tests() {

#[test]
fn rpc_tests() {
set_constants_manifest();
let t = trybuild::TestCases::new();
t.pass("tests/rpc/derive_rpc.rs");
t.pass("tests/rpc/derive_rpc_with_where.rs");
Expand All @@ -40,7 +53,9 @@ fn rpc_tests() {

#[test]
fn cli_wallet_arg_tests() {
set_constants_manifest();
let t: trybuild::TestCases = trybuild::TestCases::new();

t.pass("tests/cli_wallet_arg/derive_enum_named_fields.rs");
t.pass("tests/cli_wallet_arg/derive_struct_unnamed_fields.rs");
t.pass("tests/cli_wallet_arg/derive_struct_named_fields.rs");
Expand All @@ -51,11 +66,8 @@ fn cli_wallet_arg_tests() {

#[test]
fn constants_from_manifests_test() {
set_constants_manifest();
let t: trybuild::TestCases = trybuild::TestCases::new();
let manifest_dir = std::env::var_os("CARGO_MANIFEST_DIR").unwrap();
std::env::set_var(
"CONSTANTS_MANIFEST",
PathBuf::from(manifest_dir).join("tests"),
);

t.pass("tests/constants/create_constant.rs");
}

0 comments on commit 606d7b8

Please sign in to comment.