-
Notifications
You must be signed in to change notification settings - Fork 1
defold entrypoint #34
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
611f94a
10f7bb0
b6db7b9
6cf8c12
a291f10
99f8b5d
1aa904f
c4ec363
64048d1
d852111
fb68137
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| use crate::auth::AuthManager; | ||
| use crate::config::{self, WavedashConfig}; | ||
| use crate::config::{self, EngineKind, WavedashConfig}; | ||
| use crate::dev::entrypoint::{fetch_entrypoint_params, resolve_defold_entrypoint}; | ||
| use crate::file_staging::FileStaging; | ||
| use anyhow::Result; | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
@@ -157,14 +158,40 @@ pub async fn handle_build_push(config_path: PathBuf, verbose: bool, message: Opt | |
| anyhow::bail!("No files found in {}", upload_dir.display()); | ||
| } | ||
|
|
||
| // Get temporary R2 credentials (includes build size) | ||
| let engine_kind = wavedash_config.engine_type()?; | ||
| let engine_version = wavedash_config.engine_version(); | ||
| let entrypoint_params = match engine_kind { | ||
| Some(EngineKind::Defold) => { | ||
| let (html_path, html_relative_path) = | ||
| resolve_defold_entrypoint(&upload_dir, wavedash_config.entrypoint.as_deref())?; | ||
|
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. For Defold, Trace for a user who ran
For dev/mod.rs ( Moving |
||
| let ver = engine_version | ||
| .ok_or_else(|| anyhow::anyhow!("DEFOLD engine requires a version"))?; | ||
| Some( | ||
| fetch_entrypoint_params( | ||
| EngineKind::Defold.as_label(), | ||
| ver, | ||
| &html_path, | ||
| Some(&html_relative_path), | ||
| ) | ||
| .await?, | ||
| ) | ||
|
cloud9c marked this conversation as resolved.
|
||
| } | ||
|
Comment on lines
+178
to
+193
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.
Result: a user who sets Two ways to close this:
Either way, the gap is in |
||
| Some(EngineKind::JsDos | EngineKind::Ruffle | EngineKind::RenPy) => { | ||
| wavedash_config.executable_entrypoint_params() | ||
| } | ||
| // Explicit (not `_`) so a new EngineKind forces a decision. Godot/Unity | ||
| // params are computed server-side. | ||
| Some(EngineKind::Godot | EngineKind::Unity) => None, | ||
| None => None, | ||
| }; | ||
|
|
||
| // Get temporary R2 credentials (includes build size) | ||
| let creds = get_temp_credentials( | ||
| &wavedash_config.game_id, | ||
| engine_kind.map(|e| e.as_label()), | ||
| wavedash_config.engine_version(), | ||
| engine_version, | ||
| wavedash_config.entrypoint(), | ||
| wavedash_config.executable_entrypoint_params(), | ||
| entrypoint_params, | ||
| message.as_deref(), | ||
| total_bytes, | ||
| &api_key, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,11 @@ struct EntrypointParamsResponse { | |
| entrypoint_params: Value, | ||
| } | ||
|
|
||
| /// Locate the HTML entrypoint for engines that ship a single root export | ||
| /// (Godot, Unity): a root `index.html`, falling back to the first `.html` found. | ||
| /// Defold doesn't use this — it parses the explicit `entrypoint` from | ||
| /// wavedash.toml, since its bundles can nest the HTML under `wasm-web`/`js-web` | ||
| /// and ship more than one. No "which export is newest" guessing lives here. | ||
| pub fn locate_html_entrypoint(upload_dir: &Path) -> Option<PathBuf> { | ||
| let default_index = upload_dir.join("index.html"); | ||
| if default_index.is_file() { | ||
|
|
@@ -37,7 +42,37 @@ pub fn locate_html_entrypoint(upload_dir: &Path) -> Option<PathBuf> { | |
| None | ||
| } | ||
|
|
||
| pub async fn fetch_entrypoint_params(engine: &str, engine_version: &str, html_path: &Path) -> Result<Value> { | ||
| /// Resolve the developer-named Defold entrypoint HTML to its absolute path plus | ||
| /// normalized build-relative path. Defold names its entrypoint explicitly (a | ||
| /// bundle can ship both `wasm-web/` and `js-web/`), so there's no inference here | ||
| /// — a missing or wrong path is a clear error, not a guess. | ||
| pub fn resolve_defold_entrypoint( | ||
| upload_dir: &Path, | ||
| entrypoint: Option<&str>, | ||
| ) -> Result<(PathBuf, String)> { | ||
| let entrypoint = entrypoint.ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "Defold builds need an `entrypoint` in wavedash.toml pointing to your HTML5 export, e.g.\n entrypoint = \"wasm-web/<game>/index.html\"\n(a Defold bundle can contain both wasm-web/ and js-web/ — pick one)" | ||
| ) | ||
| })?; | ||
| let relative_path = entrypoint.replace('\\', "/"); | ||
| let html_path = upload_dir.join(&relative_path); | ||
| if !html_path.is_file() { | ||
| anyhow::bail!( | ||
| "Defold entrypoint `{}` not found under {}. Point `entrypoint` in wavedash.toml at your export's index.html.", | ||
| entrypoint, | ||
| upload_dir.display() | ||
| ); | ||
| } | ||
| Ok((html_path, relative_path)) | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| } | ||
|
Comment on lines
+49
to
+116
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.
Two concrete misconfigurations:
The user "owns their own machine" so this isn't a security exploit, but it surfaces typos and copy-paste mistakes as silent wrong-file uploads instead of clear errors. A cheap guard: canonicalize both and check containment: let html_path = upload_dir.join(&relative_path);
if !html_path.is_file() {
anyhow::bail!(/* same as today */);
}
// Reject anything that escapes upload_dir.
let canon_html = html_path
.canonicalize()
.with_context(|| format!("Failed to canonicalize {}", html_path.display()))?;
let canon_upload = upload_dir
.canonicalize()
.with_context(|| format!("Failed to canonicalize {}", upload_dir.display()))?;
if !canon_html.starts_with(&canon_upload) {
anyhow::bail!(
"Defold entrypoint `{}` resolves outside upload_dir ({}). The path must be relative to upload_dir and stay inside it.",
entrypoint,
upload_dir.display()
);
}Bonus: also reject paths whose lowercase extension isn't |
||
|
|
||
| pub async fn fetch_entrypoint_params( | ||
| engine: &str, | ||
| engine_version: &str, | ||
| html_path: &Path, | ||
| html_relative_path: Option<&str>, | ||
| ) -> Result<Value> { | ||
| let html_content = fs::read_to_string(html_path) | ||
| .with_context(|| format!("Failed to read {}", html_path.display()))?; | ||
| let api_host = config::get("api_host")?; | ||
|
|
@@ -53,6 +88,7 @@ pub async fn fetch_entrypoint_params(engine: &str, engine_version: &str, html_pa | |
| "engine": engine, | ||
| "engineVersion": engine_version, | ||
| "htmlContent": html_content, | ||
| "htmlPath": html_relative_path, | ||
|
cursor[bot] marked this conversation as resolved.
Outdated
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.
To make let mut body = serde_json::json!({
"engine": engine,
"engineVersion": engine_version,
"htmlContent": html_content,
});
if let Some(html_path) = html_relative_path {
body["htmlPath"] = serde_json::json!(html_path);
}
let response = client.post(&endpoint).json(&body).send().await
.with_context(|| "Failed to call CLI entrypoint params endpoint")?;Combined with the dev/mod.rs change to pass |
||
| })) | ||
| .send() | ||
| .await | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,11 +9,11 @@ use crate::config::{self, EngineKind, WavedashConfig}; | |
| use crate::file_staging::FileStaging; | ||
|
|
||
| mod dev_app; | ||
| mod entrypoint; | ||
| pub(crate) mod entrypoint; | ||
| mod launcher; | ||
|
|
||
| use dev_app::{ensure_dev_app, user_data_dir}; | ||
| use entrypoint::{fetch_entrypoint_params, locate_html_entrypoint}; | ||
| use entrypoint::{fetch_entrypoint_params, locate_html_entrypoint, resolve_defold_entrypoint}; | ||
| use launcher::{run_dev_app, DevAppConfig}; | ||
|
|
||
| #[derive(Debug, Deserialize)] | ||
|
|
@@ -108,20 +108,42 @@ pub async fn handle_dev(config_path: Option<PathBuf>, verbose: bool) -> Result<( | |
|
|
||
| FileStaging::prepare(&upload_dir, &wavedash_config)?; | ||
|
|
||
| let html_entrypoint = locate_html_entrypoint(&upload_dir); | ||
| let engine_version = wavedash_config.engine_version(); | ||
| let entrypoint_params = match engine_kind { | ||
| Some(EngineKind::Godot | EngineKind::Unity) => { | ||
| let engine_label = engine_kind.unwrap().as_label(); | ||
| let html_path = html_entrypoint.as_deref().ok_or_else(|| { | ||
| Some(kind @ (EngineKind::Godot | EngineKind::Unity)) => { | ||
| let engine_label = kind.as_label(); | ||
| let html_path = locate_html_entrypoint(&upload_dir).ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "No HTML file found in upload_dir; required for {} builds", | ||
| engine_label | ||
| ) | ||
| })?; | ||
| let ver = engine_version | ||
| .ok_or_else(|| anyhow::anyhow!("{} engine requires a version", engine_label))?; | ||
| Some(fetch_entrypoint_params(engine_label, ver, html_path).await?) | ||
| let html_relative_path = html_path | ||
| .strip_prefix(&upload_dir) | ||
| .unwrap_or(&html_path) | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); | ||
| Some( | ||
| fetch_entrypoint_params(engine_label, ver, &html_path, Some(&html_relative_path)) | ||
| .await?, | ||
| ) | ||
| } | ||
|
Comment on lines
+125
to
+136
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.
The Godot/Unity branch (lines 113–132) computes Suggested isolation: keep Some(kind @ (EngineKind::Godot | EngineKind::Unity)) => {
let engine_label = kind.as_label();
let html_path = locate_html_entrypoint(&upload_dir).ok_or_else(|| {
anyhow::anyhow!(
"No HTML file found in upload_dir; required for {} builds",
engine_label
)
})?;
let ver = engine_version
.ok_or_else(|| anyhow::anyhow!("{} engine requires a version", engine_label))?;
Some(fetch_entrypoint_params(engine_label, ver, &html_path, None).await?)
}This also makes the |
||
| Some(EngineKind::Defold) => { | ||
| let (html_path, html_relative_path) = | ||
| resolve_defold_entrypoint(&upload_dir, wavedash_config.entrypoint.as_deref())?; | ||
| let ver = engine_version | ||
| .ok_or_else(|| anyhow::anyhow!("DEFOLD engine requires a version"))?; | ||
| Some( | ||
| fetch_entrypoint_params( | ||
| EngineKind::Defold.as_label(), | ||
| ver, | ||
| &html_path, | ||
| Some(&html_relative_path), | ||
| ) | ||
| .await?, | ||
| ) | ||
| } | ||
| Some(EngineKind::JsDos | EngineKind::Ruffle | EngineKind::RenPy) => { | ||
| wavedash_config.executable_entrypoint_params() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ struct GamesResponse { | |
| enum EngineType { | ||
| Godot, | ||
| Unity, | ||
| Defold, | ||
| Custom, | ||
| } | ||
|
|
||
|
|
@@ -47,6 +48,8 @@ impl EngineType { | |
| match self { | ||
| EngineType::Godot => "build", | ||
| EngineType::Unity => "build", | ||
| // Defold reserves top-level build/ for its build cache, so default elsewhere. | ||
| EngineType::Defold => "dist", | ||
|
cloud9c marked this conversation as resolved.
cloud9c marked this conversation as resolved.
|
||
| EngineType::Custom => "dist", | ||
|
cloud9c marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
|
@@ -130,10 +133,25 @@ fn detect_unity(dir: &Path) -> Option<DetectedEngine> { | |
| None | ||
| } | ||
|
|
||
| /// Look for a Defold project marker. | ||
| fn detect_defold(dir: &Path) -> Option<DetectedEngine> { | ||
| if dir.join("game.project").is_file() { | ||
| return Some(DetectedEngine { | ||
| engine_type: EngineType::Defold, | ||
| version_hint: None, | ||
| }); | ||
| } | ||
|
|
||
| None | ||
| } | ||
|
cloud9c marked this conversation as resolved.
cloud9c marked this conversation as resolved.
cloud9c marked this conversation as resolved.
|
||
|
|
||
| fn detect_engine(dir: &Path) -> DetectedEngine { | ||
| if let Some(engine) = detect_godot(dir) { | ||
| return engine; | ||
| } | ||
| if let Some(engine) = detect_defold(dir) { | ||
| return engine; | ||
| } | ||
| if let Some(engine) = detect_unity(dir) { | ||
| return engine; | ||
| } | ||
|
|
@@ -233,6 +251,16 @@ fn generate_toml( | |
| let version = engine_version.unwrap_or("2022.3"); | ||
| toml.push_str(&format!("\n[unity]\nversion = \"{}\"\n", version)); | ||
| } | ||
| EngineType::Defold => { | ||
| let version = engine_version.unwrap_or("1.12.4"); | ||
| // `entrypoint` is a top-level key, so the hint must precede the [defold] | ||
| // table. Left commented — the dev fills in the export they want to ship; | ||
| // `wavedash dev` / `build push` errors clearly until they do. | ||
| toml.push_str( | ||
| "\n# Defold HTML5 bundles can contain both wasm-web/ and js-web/ folders.\n# Set `entrypoint` (top-level) to the index.html you want to ship, e.g.:\n# entrypoint = \"wasm-web/<game>/index.html\"\n", | ||
| ); | ||
| toml.push_str(&format!("\n[defold]\nversion = \"{}\"\n", version)); | ||
| } | ||
|
Comment on lines
+254
to
+263
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.
The error message is clear once you trigger it, but the UX cliff is:
Two cheaper alternatives, in order of effort:
Right now 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.
The comment at lines 256–258 documents the design ("the dev fills in the export they want to ship;
This is the only engine where the next step the outro promises doesn't work without manual file editing. Two cheap mitigations, in order of effort:
Either is fine — the goal is just to make the cliff visible before the user hits it. Acknowledged in the previous review pass; flagging once more since the latest commit ("edge cases") had room to address it. |
||
| EngineType::Custom => { | ||
| toml.push_str("\nentrypoint = \"index.html\"\n"); | ||
| } | ||
|
|
@@ -279,7 +307,7 @@ pub async fn handle_init() -> Result<()> { | |
| let current_dir = std::env::current_dir()?; | ||
| let detected = detect_engine(¤t_dir); | ||
|
|
||
| // Only prompt for version when we detect Godot/Unity. | ||
| // Only prompt for version when we detect a first-class engine. | ||
| // For web builds (threejs, phaser, custom, etc.) no engine config is needed. | ||
| let engine_version: Option<String> = match &detected.engine_type { | ||
| EngineType::Godot => { | ||
|
|
@@ -302,6 +330,13 @@ pub async fn handle_init() -> Result<()> { | |
| Some(version) | ||
| } | ||
| } | ||
| EngineType::Defold => { | ||
| let version: String = cliclack::input("Defold version") | ||
| .placeholder("1.12.4") | ||
| .default_input("1.12.4") | ||
| .interact()?; | ||
| Some(version) | ||
| } | ||
|
cloud9c marked this conversation as resolved.
|
||
| EngineType::Custom => None, | ||
| }; | ||
|
|
||
|
|
||
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.
upload_diris still not canonicalized inhandle_build_push— onlyresolve_defold_entrypoint's internal canonicalize-containment check runs, whiledev/mod.rs:101canonicalizes once and the rest of the flow operates on a clean absolute path.This was raised in a prior review and is moot for path-escape safety —
resolve_defold_entrypointre-canonicalizes both sides before itsstarts_withcheck (entrypoint.rs:92-104), so escapes are rejected. But the broader symmetry concern still applies:html_pathat entrypoint.rs:106 is the un-canonicalizedupload_dir.join(relative_path). It's read viafs::read_to_string(fine), but it flows intofetch_entrypoint_paramsand any error messages downstream — the two flows disagree on the textual path for the same project.wavedash devandwavedash build pushfor the same misconfiguration —devreports a/private/var/...absolute path;build pushreports./build/default/.... That asymmetry makes user-reported failures harder to triage.R2Uploaderat line 215 walks the non-canonical path; pre-PR behavior, but worth confirming the new Defold flow doesn't introduce sensitivity to symlinks underupload_dir.Restoring the canonicalize block (the same one added in
a291f10and removed in1aa904f) is the simplest fix and keeps the two flows symmetric:(requires restoring
use anyhow::Context;.)