Say on the card when a project's own file was ignored
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 loses that project's components, strip, staleWhen and resources at once. Nothing looks broken afterwards: the card becomes a project that declares nothing, which is the ordinary case, and the only thing saying otherwise was a line in this server's log on a machine the person holding the phone cannot read. project_config now answers a ProjectFile carrying the parse error beside the declaration, and AppEntry::declaration_state returns both halves from one read -- the manifest wants both, and the file is on that path. The card draws its own sentence about what it means and what to do, then the parser's own words, which name the file, the position and the offending field; that half is 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 -- asserted in the test, along with the error naming both the field and the file. Seen on the emulator both ways: a project whose file gained a field from the future says so under its action row, and the whole message goes when the file parses again. Raised by the tdep-survey session after its alsoWatch commit hit exactly this against the older binary running here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
c7b3af4861
commit
0b7164bb30
8 files changed
+210
-28
No files matched your search
@@ -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 {
|
||||
|
||||
+44
-12
@@ -756,21 +756,53 @@ pub fn render_request(components: &[Component]) -> Result<String, ron::Error> {
|
||||
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<String>,
|
||||
}
|
||||
|
||||
/// 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
|
||||
|
||||
+1
-1
@@ -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()
|
||||
|
||||
+93
-13
@@ -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<Vec<Component>>,
|
||||
/// 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<String>,
|
||||
}
|
||||
|
||||
/// 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<Vec<Component>> {
|
||||
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: `<repo>/app` is the
|
||||
/// self entry's project, and `<repo>` is what gets pulled.
|
||||
fn self_checkout(root: &Path) -> PathBuf {
|
||||
|
||||
+15
-1
@@ -546,6 +546,18 @@ struct ManifestApp {
|
||||
/// something the keys don't say.
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pending_declaration: Option<String>,
|
||||
/// 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<String>,
|
||||
}
|
||||
|
||||
/// One build found under a project. Which of them a device wants is that
|
||||
@@ -643,7 +655,8 @@ async fn describe(state: &Arc<AppState>, entry: &AppEntry) -> Result<ManifestApp
|
||||
// Read before the components, because each of them reports whether it
|
||||
// can be built and none of them can while this is set: an unaccepted
|
||||
// declaration is exactly the command that must not run.
|
||||
let pending = entry.pending_declaration();
|
||||
let declaration = entry.declaration_state();
|
||||
let pending = declaration.pending;
|
||||
let mut components = Vec::with_capacity(entry.components.len());
|
||||
for component in &entry.components {
|
||||
components.push(
|
||||
@@ -693,6 +706,7 @@ async fn describe(state: &Arc<AppState>, entry: &AppEntry) -> Result<ManifestApp
|
||||
pending_declaration: pending
|
||||
.as_ref()
|
||||
.and_then(|components| crate::config::render_request(components).ok()),
|
||||
declaration_error: declaration.error,
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user