-
Notifications
You must be signed in to change notification settings - Fork 297
Run a Wasm file as an application without a manifest #2479
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ use spin_manifest::schema::v2::AppManifest; | |
| pub enum AppSource { | ||
| File(PathBuf), | ||
| OciRegistry(String), | ||
| BareWasm(PathBuf), | ||
|
Collaborator
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. Should we add an explicit CLI arg (e.g.
Collaborator
Author
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. Yes, I had wondered about this. I'm concerned it would make the docs a bit confusing, given that
Collaborator
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. Maybe it would be better to instead have an orthogonal option to differentiate between manifestful and manifestless sources. Anyway, that ought to be backward compatible with this so definitely not a blocker here. |
||
| Unresolvable(String), | ||
| None, | ||
| } | ||
|
|
@@ -31,7 +32,13 @@ impl AppSource { | |
|
|
||
| pub fn infer_file_source(path: impl Into<PathBuf>) -> Self { | ||
| match spin_common::paths::resolve_manifest_file_path(path.into()) { | ||
| Ok(file) => Self::File(file), | ||
| Ok(file) => { | ||
| if is_wasm_file(&file) { | ||
| Self::BareWasm(file) | ||
| } else { | ||
| Self::File(file) | ||
| } | ||
| } | ||
| Err(e) => Self::Unresolvable(e.to_string()), | ||
| } | ||
| } | ||
|
|
@@ -58,11 +65,17 @@ impl AppSource { | |
| } | ||
| } | ||
|
|
||
| fn is_wasm_file(path: &Path) -> bool { | ||
| let extn = path.extension().and_then(std::ffi::OsStr::to_str); | ||
| extn.is_some_and(|e| e == "wasm" || e == "wat") | ||
| } | ||
|
|
||
| impl std::fmt::Display for AppSource { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| Self::File(path) => write!(f, "local app {}", quoted_path(path)), | ||
| Self::OciRegistry(reference) => write!(f, "remote app {reference:?}"), | ||
| Self::BareWasm(path) => write!(f, "Wasm file {}", quoted_path(path)), | ||
| Self::Unresolvable(s) => write!(f, "unknown app source: {s:?}"), | ||
| Self::None => write!(f, "<no source>"), | ||
| } | ||
|
|
@@ -77,6 +90,9 @@ pub enum ResolvedAppSource { | |
| manifest_path: PathBuf, | ||
| manifest: AppManifest, | ||
| }, | ||
| BareWasm { | ||
| wasm_path: PathBuf, | ||
| }, | ||
| OciRegistry { | ||
| locked_app: LockedApp, | ||
| }, | ||
|
|
@@ -85,17 +101,20 @@ pub enum ResolvedAppSource { | |
| impl ResolvedAppSource { | ||
| pub fn trigger_types(&self) -> anyhow::Result<Vec<&str>> { | ||
| let types = match self { | ||
| ResolvedAppSource::File { manifest, .. } => { | ||
| manifest.triggers.keys().collect::<HashSet<_>>() | ||
| } | ||
| ResolvedAppSource::File { manifest, .. } => manifest | ||
| .triggers | ||
| .keys() | ||
| .map(|s| s.as_str()) | ||
| .collect::<HashSet<_>>(), | ||
| ResolvedAppSource::OciRegistry { locked_app } => locked_app | ||
| .triggers | ||
| .iter() | ||
| .map(|t| &t.trigger_type) | ||
| .map(|t| t.trigger_type.as_str()) | ||
| .collect::<HashSet<_>>(), | ||
| ResolvedAppSource::BareWasm { .. } => ["http"].into_iter().collect(), | ||
| }; | ||
|
|
||
| ensure!(!types.is_empty(), "no triggers in app"); | ||
| Ok(types.into_iter().map(|t| t.as_str()).collect()) | ||
| Ok(types.into_iter().collect()) | ||
| } | ||
| } | ||
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 would feel slightly better if we used a tempdir here. It shouldn't make any difference as the feature currently works, but as features are added I wouldn't want to accidentally expose content from the cwd.
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.
Actually, I believe you already have
working_dirwhere this function is called; could probably just pass that through.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 is the directory relative to which to resolve the Wasm file reference (and, perhaps, in future, other paths). If we use
working_dirthen we have to copy the Wasm file first (before copying it again), and any file-related errors will show unexpected locations.The alternative is for
app_rootto be the directory containing the Wasm file, and to overridewasm_pathto be only the name of the Wasm file (so that the relative reference resolves correctly), but I'm not sure that's going to be helpful with hypothetical future features where the user will expect paths entered on the CLI to be resolved relative to the current directory. Of course it's hard to know because we haven't defined those features yet!We could also do a more invasive change that threads multiple "app roots" through the loader, or redefines the app root, or absolutises everything earlier. Or we can have a separate loader path for much longer than this PR does it, e.g. doing any copies needed then jumping straight to building the lockfile instead of leaning on the existing manifestful path.
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.
Sounds complicated. I retract my feelings.