diff --git a/AGENTS.md b/AGENTS.md index a420130..8aef175 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -255,6 +255,26 @@ mutable at runtime from the phone. that silence. It is part of the acceptance gate like every other declared field -- `same_declaration` destructures exhaustively, which is what forced the decision when the field was added. +- **A declaration that cannot be read says so on the card.** RON's + `deny_unknown_fields` discards the *whole* declaration rather than the + key it did not recognise, so a machine that takes a project's commit + before it takes a dev-updater new enough to understand it loses that + project's components, `strip`, `staleWhen` and resources at once -- and + the card does not look broken, it looks like a project that declares + nothing, which is the ordinary case. That was the only difference + between the two states: a line in this server's log, on a machine the + person holding the phone cannot read. `config::project_config` now + answers a `ProjectFile` carrying the parse error beside the + declaration, `AppEntry::declaration_state` returns both halves from one + read (the manifest wants both, and the file is on that path), and the + card draws its own sentence about what it means followed by the + parser's own words -- selectable, since the fix happens on the other + machine. Nothing is *blocked* by it: what was already accepted is what + runs, so an unrelated typo cannot stop a project that was working. + Worst on the self entry, where `reconcile_self` derives the row from + that file at every startup: unreadable, and this server's own card + silently loses its server component and its build commands rather than + merely stopping noticing changes. - **The debug keystore is not `~/.android/debug.keystore`** -- that is the last of five places, and `sdk::android_prefs_dirs` resolves it the way AGP does, because the only thing that makes it the right key is being diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt index d54848a..6073403 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt @@ -63,6 +63,14 @@ data class ManifestEntry( // Null once accepted, or for a project that asks for nothing. While it // is set the server runs no build step for this app at all. val pendingDeclaration: String?, + // Why this project's own .dev-updater.ron was ignored, when it could + // not be read. Null covers both "it parsed" and "there is no file", + // which are the same thing to a reader -- what this exists for is the + // third case, where the file is there, says something, and none of it + // is being used. An unknown field discards the whole declaration + // rather than the key it did not recognise, so the card silently + // becomes an emptier project instead of a broken one. + val declarationError: String?, ) // A project's checkout, all read locally on the server. @@ -340,6 +348,7 @@ private fun readEntry(entry: JSONObject): ManifestEntry { checkError = entry.optString("checkError").ifEmpty { null }, checkPending = entry.optBoolean("checkPending", false), pendingDeclaration = entry.optString("pendingDeclaration").ifEmpty { null }, + declarationError = entry.optString("declarationError").ifEmpty { null }, components = (0 until (components?.length() ?: 0)).map { j -> val component = components!!.getJSONObject(j) diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt index 83efd04..96855b5 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -1919,6 +1919,29 @@ private fun AppCard( OutputText(reason, style = MaterialTheme.typography.bodySmall) } } + // Said here, with the project's identity, because it is about + // this project's own file rather than about any one component + // -- and because what it costs is invisible further down: the + // file is ignored *whole*, so the card quietly becomes an + // emptier project, with components, strip and resources it + // asked for simply not there. Nothing else on the card can say + // that, since a project that declares nothing looks exactly + // the same. + // + // Two parts, and the split is the usual one: this app's own + // sentence for what it means and what to do, then the parser's + // own words -- which name the file, the line and the field -- + // selectable, because fixing it happens on the other machine. + entry.declarationError?.let { reason -> + Text( + "This project's own file was ignored, so nothing it asks for is being " + + "read. Fix it on the build machine, or update Dev Updater if the file " + + "uses something newer than it knows.", + style = MaterialTheme.typography.bodySmall, + color = ActionTone.Caution.color, + ) + OutputText(reason, style = MaterialTheme.typography.bodySmall) + } } } diff --git a/server/src/build_state.rs b/server/src/build_state.rs index ca4b4cb..3f0f35f 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -1411,7 +1411,11 @@ mod tests { let project = clone.clone(); state.pull_and_build( false, - move || crate::config::project_config(&project).matches_accepted(&accepted, None), + move || { + crate::config::project_config(&project) + .declaration + .matches_accepted(&accepted, None) + }, Arc::new(|_: &str, _: String| {}), ); while state.status().building { diff --git a/server/src/config.rs b/server/src/config.rs index 35e9d74..50a9b79 100644 --- a/server/src/config.rs +++ b/server/src/config.rs @@ -756,21 +756,53 @@ pub fn render_request(components: &[Component]) -> Result { wg_app_link::format::render(&Request { components }) } +/// A project's own file: what it says, and why it could not be read. +/// +/// Two fields rather than a `Result` because the caller needs both halves +/// at once. Every caller carries on with an empty declaration -- one +/// project's typo must not take the whole app list down -- and the card +/// still has to be able to say that is what happened, which a discarded +/// error cannot do. +#[derive(Debug, Default)] +pub struct ProjectFile { + /// Empty for a project that says nothing *and* for one whose file + /// could not be read. [`Self::error`] is what tells those apart. + pub declaration: Declaration, + /// Why the file was ignored, in the parser's own words -- position, + /// the offending field, and what was expected there. + pub error: Option, +} + /// What `project` says about itself, or an empty declaration for a project /// that says nothing -- which is the normal case. /// -/// A file that doesn't parse is logged and treated as saying nothing -/// rather than as an error: this runs while rebuilding the entry list, and -/// one project's typo must not take the whole app list down with it. -pub fn project_config(project: &Path) -> Declaration { +/// A file that doesn't parse is treated as saying nothing rather than as +/// an error, because this runs while rebuilding the entry list. What it is +/// *not* is silent: an unknown field discards the whole declaration, not +/// the key it did not recognise, so a machine that takes a project's +/// commit before it takes a dev-updater that understands it loses that +/// project's build steps, its `strip`, its `staleWhen` and its resources +/// at once. Nothing about the card looks broken afterwards -- it becomes +/// a quieter, emptier project -- which is why the reason travels to the +/// phone rather than only to this server's log. +pub fn project_config(project: &Path) -> ProjectFile { let path = project.join(PROJECT_CONFIG_FILE); let Ok(text) = std::fs::read_to_string(&path) else { - return Declaration::default(); + return ProjectFile::default(); }; - wg_app_link::format::parse(&text).unwrap_or_else(|err| { - tracing::warn!("ignoring {}: {err}", path.display()); - Declaration::default() - }) + match wg_app_link::format::parse(&text) { + Ok(declaration) => ProjectFile { + declaration, + error: None, + }, + Err(err) => { + tracing::warn!("ignoring {}: {err}", path.display()); + ProjectFile { + declaration: Declaration::default(), + error: Some(format!("{}: {err}", path.display())), + } + } + } } /// The inverse of [`expand_tilde`], for paths shown on a phone. @@ -1060,7 +1092,7 @@ mod tests { fn reads_the_build_step_a_project_asks_for() { let dir = tempfile::tempdir().expect("tempdir"); // The normal case: no file at all, which is not an error. - assert!(project_config(dir.path()).components.is_empty()); + assert!(project_config(dir.path()).declaration.components.is_empty()); std::fs::write( dir.path().join(PROJECT_CONFIG_FILE), @@ -1069,7 +1101,7 @@ mod tests { components: [Apk(name: \"app\", build: \"./build-apk.sh\")],\n", ) .expect("write"); - let asked = project_config(dir.path()); + let asked = project_config(dir.path()).declaration; assert!(!asked.git_pull); let [component] = &asked.components[..] else { panic!("one component") @@ -1087,7 +1119,7 @@ mod tests { "components: [Apk(name:", ) .expect("write"); - assert!(project_config(dir.path()).components.is_empty()); + assert!(project_config(dir.path()).declaration.components.is_empty()); } /// Both ways of writing a command mean the same argv, and the one with diff --git a/server/src/main.rs b/server/src/main.rs index 4e4a7ce..58081a3 100644 --- a/server/src/main.rs +++ b/server/src/main.rs @@ -170,7 +170,7 @@ struct Args { /// declaration -- so a bootstrap script cannot drift out of step with them /// the way a second copy of the arguments would. fn drive_own_service(self_project: &Path, subcommand: &str) -> Result<()> { - let declaration = config::project_config(self_project); + let declaration = config::project_config(self_project).declaration; let component = declaration .components .iter() diff --git a/server/src/registry.rs b/server/src/registry.rs index d0f5bf5..b77fa8f 100644 --- a/server/src/registry.rs +++ b/server/src/registry.rs @@ -33,6 +33,17 @@ pub const SELF_KEY: &str = "updater"; const SELF_PACKAGE: &str = "com.example.devupdater"; pub(crate) const SELF_LABEL: &str = "Dev Updater"; +/// What a project's own `.dev-updater.ron` says as of now, which is two +/// separate things a card has to be able to draw differently. See +/// [`AppEntry::declaration_state`]. +pub struct DeclarationState { + /// What it asks for that has not been accepted, when that is anything. + pub pending: Option>, + /// Why the file was ignored. `None` covers both "it parsed" and "there + /// is no file", which are the same thing to everything downstream. + pub error: Option, +} + /// One servable app, resolved from config (or built in, for the self /// entry). Cheap to clone-by-`Arc` and handed to request handlers whole. pub struct AppEntry { @@ -161,21 +172,43 @@ impl AppEntry { /// Cheap enough for the manifest path: one small read per app, beside /// the `git status` that path already runs for each of them. pub fn pending_declaration(&self) -> Option> { + self.declaration_state().pending + } + + /// Both halves of what this project's own file says right now: what it + /// is asking for that nobody has accepted, and why it could not be + /// read at all. + /// + /// One read answering both, because the manifest wants both and the + /// file is on that path. + /// + /// The two are not alternatives. A file that does not parse is asking + /// for nothing -- so `pending` is `None`, and the card would otherwise + /// be indistinguishable from a project that declares nothing at all, + /// which is the ordinary case. It is the `error` that separates them, + /// and without it the only sign was a line in this server's log. + pub fn declaration_state(&self) -> DeclarationState { // This server's own project is accepted by construction (see // `reconcile_self`), and the row saying so is only re-derived at // startup -- so between a pull that changes the declaration and the // restart that follows it, the two can differ without that meaning - // anything is waiting to be accepted. - if self.built_in { - return None; + // anything is waiting to be accepted. An unreadable file is still + // worth saying, and matters more here than anywhere: this row's + // components come *from* that file at startup, so a declaration + // this server cannot parse is a card that has lost its server + // component and its build commands rather than one that has + // stopped noticing changes. + let file = crate::config::project_config(&self.project_path); + let pending = (!self.built_in + && !file.declaration.components.is_empty() + && !file + .declaration + .matches_accepted(&self.components, self.resources.as_ref())) + .then_some(file.declaration.components); + DeclarationState { + pending, + error: file.error, } - let declared = crate::config::project_config(&self.project_path); - if declared.components.is_empty() - || declared.matches_accepted(&self.components, self.resources.as_ref()) - { - return None; - } - Some(declared.components) } /// This project's resources, as something to read, when it declares @@ -540,7 +573,7 @@ impl AppState { // project is still added -- there is simply nothing installed for // it yet, which is the honest answer and is what lets a project be // added in order to run the build that produces its first APK. - let declared = crate::config::project_config(&project); + let declared = crate::config::project_config(&project).declaration; let (label, package) = match discover::find_apks(&project).into_iter().next() { Some(apk) => { let info = crate::apkinfo::read(&apk.path)?; @@ -647,7 +680,7 @@ impl AppState { let entry = self .entry(key) .with_context(|| format!("no app named {key}"))?; - let declared = crate::config::project_config(&entry.project_path); + let declared = crate::config::project_config(&entry.project_path).declaration; if declared.components.is_empty() { bail!( "{} no longer asks for anything -- its {} is gone or unreadable", @@ -712,7 +745,7 @@ impl AppState { /// doing the gating, so whatever arrives is already trusted by the time it /// could run. fn reconcile_self(config: &mut Config, self_project: &Path) -> bool { - let declared = crate::config::project_config(self_project); + let declared = crate::config::project_config(self_project).declaration; // Asked of the project itself, not of its parent. `git status` answers // from any subdirectory, so the two agree whenever `app/` is really // there -- and differ in the one case that matters: a working directory @@ -1168,6 +1201,53 @@ mod tests { ); } + /// A file this server cannot read says nothing, which is exactly what + /// a project with no file says -- so the reason has to travel + /// separately, or the two are the same card. + /// + /// The shape that produces it: an unknown field discards the *whole* + /// declaration rather than the key it did not recognise, so a machine + /// that takes a project's commit before it takes a dev-updater new + /// enough to understand it loses that project's build steps, `strip`, + /// `staleWhen` and resources at once, with nothing on screen changed + /// but the absence. + #[test] + fn a_declaration_that_cannot_be_read_says_so_rather_than_saying_nothing() { + let dir = tempfile::tempdir().expect("tempdir"); + write_request(dir.path(), "./build.sh"); + let entry = entry_for(dir.path(), asking_for("./build.sh")); + let state = entry.declaration_state(); + assert_eq!(state.pending, None, "accepted to begin with"); + assert_eq!(state.error, None, "and readable"); + + // A field from a later version of dev-updater than this one. + std::fs::write( + dir.path().join(crate::config::PROJECT_CONFIG_FILE), + "components: [Apk(name: \"app\", build: \"./build.sh\", fromTheFuture: true)],\n", + ) + .expect("write"); + + let state = entry.declaration_state(); + assert_eq!( + state.pending, None, + "a file that parsed into nothing is asking for nothing", + ); + let error = state.error.expect("the reason it was ignored"); + assert!( + error.contains("fromTheFuture"), + "the field that stopped it: {error}", + ); + assert!( + error.contains(crate::config::PROJECT_CONFIG_FILE), + "and which file it was in: {error}", + ); + assert_eq!( + entry.components, + asking_for("./build.sh"), + "what was already accepted still runs -- this is about noticing, not about stopping", + ); + } + /// A checkout laid out the way this repository is: `/app` is the /// self entry's project, and `` is what gets pulled. fn self_checkout(root: &Path) -> PathBuf { diff --git a/server/src/routes.rs b/server/src/routes.rs index 0478023..8bcf695 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -546,6 +546,18 @@ struct ManifestApp { /// something the keys don't say. #[serde(skip_serializing_if = "Option::is_none")] pending_declaration: Option, + /// Why this project's own `.dev-updater.ron` was ignored, when it + /// could not be read. + /// + /// Sent because the card is otherwise indistinguishable from a project + /// that declares nothing, which is the ordinary case: an unknown field + /// discards the whole declaration rather than the key it did not + /// recognise, so a machine that takes a project's commit before it + /// takes a dev-updater that understands it quietly becomes a different, + /// emptier project. Nothing looks broken, and the only sign was a line + /// in a log on a machine the person holding the phone cannot read. + #[serde(skip_serializing_if = "Option::is_none")] + declaration_error: Option, } /// One build found under a project. Which of them a device wants is that @@ -643,7 +655,8 @@ async fn describe(state: &Arc, entry: &AppEntry) -> Result, entry: &AppEntry) -> Result