Skip to content

appmanifest: validate fap_version type; accept int, reject float (refs ufbt#52) - #4375

Open
hypery11 wants to merge 1 commit into
flipperdevices:devfrom
hypery11:fix/ufbt-52-fap-version-validation
Open

appmanifest: validate fap_version type; accept int, reject float (refs ufbt#52)#4375
hypery11 wants to merge 1 commit into
flipperdevices:devfrom
hypery11:fix/ufbt-52-fap-version-validation

Conversation

@hypery11

Copy link
Copy Markdown

Refs flipperdevices/flipperzero-ufbt#52.

What this fixes

When a user writes fap_version=1.0 (a Python float literal) in application.fam, the current FlipperApplication.__post_init__ silently lets that survive — it only normalises fap_version if it's a str. The TypeError only surfaces deep in the build step, in scripts/fbt_tools/fbt_extapps.py:_setup_app_env:

("FAP_VERSION", f'\\"{".".join(map(str, self.app.fap_version))}\\"'),

…with the cryptic TypeError: 'float' object is not iterable, because float itself isn't iterable.

The original reporter on ufbt#52 proposed patching fbt_extapps.py, but that doesn't help — map(str, 1.0) raises the same TypeError. The right place to catch it is the manifest schema validation, where the error message can name the actual problem.

What the patch does

In FlipperApplication.__post_init__, after the existing appid regex check:

  1. Accept intfap_version=1 becomes (1, 0). Matches the common shorthand. bool is explicitly excluded (it subclasses int).
  2. Reject float with a clear message explaining why floats are unsafe for version numbers (1.10 becomes 1.1 at the Python literal level).
  3. Reject other unexpected types (list, etc.) at the manifest layer, naming the type received.

The existing str and tuple paths are unchanged.

Verification

Ran 7 cases against the patched dataclass:

default            -> OK  fap_version = (0, 1)
str "1.0"          -> OK  fap_version = (1, 0)
tuple (3,7)        -> OK  fap_version = (3, 7)
int 1              -> OK  fap_version = (1, 0)         # new
float 1.0          -> REJECT: Invalid fap_version 1.0: floats are not accepted...
bool True          -> REJECT: fap_version must be a string like '1.2' or a tuple like (1, 2), got bool: True
list [1,0]         -> REJECT: fap_version must be a string like '1.2' or a tuple like (1, 2), got list: [1, 0]

All previously-valid inputs (str, tuple, default) still pass through unchanged.

Scope

One file, +30 / -2 lines, no public API change. The new behaviour is strictly additive (int accepted) and stricter (float/list/bool now rejected at manifest time instead of crashing later). Existing valid manifests are unaffected.

@turbospok turbospok added the Build System & Scripts fbt, scripts and toolchain-related label Jun 26, 2026
@chrisdebian

Copy link
Copy Markdown

This one's pure Python (the manifest validation logic), so fully testable without hardware. Extracted __post_init__'s validation logic and ran it against 8 edge cases, including the subtlety the comments call out — bool being an int subclass in Python. Confirmed both True and False are correctly rejected as bad-type rather than silently coerced to 1/0. Floats, valid/invalid strings, tuples, and None all behaved exactly as documented. Nice catch on the float trailing-zero issue too (flipperzero-ufbt#52) — clear error message.

@munzzyy

munzzyy commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

The float rejection is the right call, and your diagnosis is right too: patching fbt_extapps.py wouldn't have helped, since map(str, 1.0) raises the same TypeError. But the new elif at scripts/fbt/appmanifest.py:143 catches more than bad input.

A list works today. fap_version=[2, 3] goes through ".".join(map(str, ...)) at scripts/fbt_tools/fbt_extapps.py:61 and fap_version[0] & 0xFFFF at scripts/fbt/elfmanifest.py:66 exactly like the tuple does, producing the same FAP_VERSION define and the same packed int. I ran both on dev with and without the patch: before, [2, 3] builds fine; after, it's a hard FlipperManifestException.

Six apps currently in flipper-application-catalog use that form. I checked every manifest at its pinned commit_sha (390 of 413 resolved, the rest are dead repos or moved commits):

The catalog builds those with ufbt faps (tools/build.py:157) and ufbt's SConstruct imports this same dataclass (scripts/ufbt/SConstruct:46), so I'd expect all six to stop building once this lands in a released SDK. I traced the import chain rather than running a catalog build, so treat that last step as strongly-suspected rather than measured.

documentation/AppManifests.md:51 only documents string and tuple, so you could argue those six were always out of spec. That's fair, but it's a call the maintainers should make deliberately, and the PR description says existing valid manifests are unaffected, which isn't quite true. Keeping lists costs one line:

        elif isinstance(self.fap_version, (tuple, list)):
            self.fap_version = tuple(self.fap_version)
        else:
            raise FlipperManifestException(
                f"fap_version must be a string like '1.2' or a tuple like (1, 2), "
                f"got {type(self.fap_version).__name__}: {self.fap_version!r}"
            )

That also cleans up something the current patch leaves as-is: with a list, fap_version stays a list on the object, so its type after __post_init__ depends on how it was written.

Separate and smaller: the len < 2 check at line 141 only runs on the string path. fap_version=(1,) still gets through and dies at elfmanifest.py:67 with IndexError: tuple index out of range, which is the same cryptic-failure-deep-in-the-build shape you're fixing. Moving that length check below the type normalisation would cover every path for free.

Last thought, purely about getting this merged: the float rejection fixes ufbt#52, the int shorthand is a new feature. They've been sitting together since May. Splitting the float part out might move faster on its own.

Not a maintainer, just read through it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Build System & Scripts fbt, scripts and toolchain-related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants