Conversation
| name = "craft-basics" | ||
| module_hash = "d7edcb9150d12af76a54fbbac7b00b8bb24fab61bc1395b65da47547ad5d1b42" | ||
|
|
||
| [[classes]] |
There was a problem hiding this comment.
does this PR also add the ability to create pexe's without declaring a new class?
There was a problem hiding this comment.
That concept doesn't make any sense to me. Declaring new classes is what pexes are for!
There was a problem hiding this comment.
in PAC, agents negotiate a pexe on the fly which is "throwaway", its just meant to represent a swap of 2 objects
so all it has is
fn Swap(){
rekeyA()
rekeyB()
}
| # The pin is stamped by `pexe build`, which resolves the import among | ||
| # already-built archives (build craft-basics first). | ||
| [[imports]] | ||
| name = "craft-basics" |
There was a problem hiding this comment.
without a path to the code how is the code resolved? does it loop in the directory? curious why you went for this approach vs making the user specify the path
There was a problem hiding this comment.
ahh I see there is --deps so this is kind of like specifying a path but not at the pexe level
| .collect(); | ||
| entries.sort(); | ||
| thread_local! { | ||
| /// Per-thread module cache, keyed by validated manifest hash. |
There was a problem hiding this comment.
Does this get refreshed on new installs? ie when reload_catalog is called in driver
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
Overall LGTM! I've left a few comments with various suggestions
| # The pin is stamped by `pexe build`, which resolves the import among | ||
| # already-built archives (build craft-basics first). | ||
| [[imports]] | ||
| name = "craft-basics" |
There was a problem hiding this comment.
This name is the alias right? I would suggest naming this field alias. I see this value stored in a variable named alias in the implementation. Otherwise one may think that the name must match the original module name.
| //! The compiled [`sdk::SdkModule`] is not kept — it holds a `Rc<Engine>` and is | ||
| //! therefore `!Send`. `execute_action` re-loads the script from its stored bytes | ||
| //! on demand, matching the per-call pattern used before. | ||
| //! [`sdk::SdkModule`] is not `Send`, so execution uses a thread-local cache of |
There was a problem hiding this comment.
Rhai has a feature to make its types thread-safe (Send). If we need it we could enable it. Doing that would require replacing a bunch of Rcs for Arcs (I'm not sure about the performance impact of that change).
That would probably allow making the SdkModule Send.
| } | ||
| let sdk = Sdk::default(); | ||
| let mut resolver = plugin_resolver(&sdk, &self.plugins); | ||
| let module = resolver |
There was a problem hiding this comment.
This function caches the requested module, but it doesn't cache the imported modules right?
So if I have these two module graphs:
A <- B(A imports B)C <- B(C imports B)
And I call compiled_module(A), B and A are compiled and then COMPILED_MODULES = {A}
Then I call compiled_module(C), B and C are compiled (so B is compiled again) and then COMPILED_MODULES = {A, C}.
I wonder if a small change could allow us to store B in the thread-local cache, and have the plugin resolver also use it, so that in the scenario I described, B is not compiled twice.
|
|
||
| # Resolve [[imports]] against archives in an extra directory, searched | ||
| # before target/pexe and the install dir | ||
| cargo run -p pexe --release -- build --deps /path/to/archives examples/craft-totem |
There was a problem hiding this comment.
Can we add multiple --deps flags in the cli call?
I believe the answer is yes, as deps is a vector in the cli struct.
| } | ||
| } | ||
| map | ||
| module.module_aliases() |
There was a problem hiding this comment.
Maybe build_alias_map can be deleted? Writing module.module_aliases() is similarly short.
| impl fmt::Display for ActionObjectRef { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match &self.defining { | ||
| Some(defining) => write!(f, "{defining:?}::{}", self.class), |
There was a problem hiding this comment.
DefiningModule contains SdkModule. Is it a good idea to do a Debug display here? Won't it be too crowded with data?
| meta.total_outputs.extend( | ||
| sub.total_outputs | ||
| .iter() | ||
| .map(|object_ref| object_ref.qualified_by(defining.as_ref())), |
There was a problem hiding this comment.
I just realized that ActionObjectRef has defining: Option<DefiningModule>, and because it's optional we need to handle both cases in several places.
What if the action when called from the module that defines it also contains DefiningModule? Internally it has an Rc<SdkModule> so having copies is cheap. And then we don't need to handle 2 different cases.
To be consistent, we could have a special alias to refer to the local module, like root? I've seen the mention of root as the module name of the module where actions are defined.
| if alias.is_empty() | ||
| || alias.starts_with(|c: char| c.is_ascii_digit()) | ||
| || alias | ||
| .chars() | ||
| .any(|c| !(c.is_ascii_alphanumeric() || c == '_')) | ||
| { | ||
| return Err(anyhow!( | ||
| "import alias {:?} is not a valid podlang module alias", | ||
| import.alias | ||
| )); | ||
| } | ||
| if alias == "tx" { | ||
| return Err(anyhow!( | ||
| "import alias {:?} collides with the reserved tx module alias", | ||
| import.alias | ||
| )); | ||
| } | ||
| if PODLANG_RESERVED_WORDS.contains(&alias.as_str()) { | ||
| return Err(anyhow!( | ||
| "import alias {:?} maps to {alias:?}, which podlang reserves", | ||
| import.alias | ||
| )); | ||
| } |
There was a problem hiding this comment.
it would be nice to move all this code to a validate_alias function
| } | ||
| fn collect_module_aliases(&self, aliases: &mut HashMap<Hash, String>) { | ||
| // Traverse children first so direct aliases take precedence. | ||
| for import in &self.imports { |
There was a problem hiding this comment.
Question for myself:
Imports are declared in the manifest.
Where are dependencies declared?
| for declared in &manifest.imports { | ||
| let import = imports | ||
| .iter() | ||
| .find(|import| import.alias == declared.name) |
There was a problem hiding this comment.
From an earlier comment, I think declared.alias would make this more clear.
This branch has not been deployed
3 participants
Closes #113
Cross-module actions. Pexe plugins can now declare dependencies on other pexe plugins.
A dependency declaration has two parts: a module hash, and an alias. The alias is the name which is used locally to refer to the dependency - it is a purely local alias which is used in the Rhai script to refer to the dependency's classes/actions by name, and in the generated Podlang. Aliases must be locally unique (a plugin cannot use the same alias for two different imports) but does not need to be globally unique (two different plugins might use the same alias to refer to different dependencies, including different "versions" of the "same" plugin).
In the future we might want to introduce a package registry, which would give us globally unique names, and semantic version numbers for plugins. However, a content-based approach works for now.