Skip to content

Commit 0ae218f

Browse files
alexmmillerclaude
andcommitted
ADFA-5548: Fix ten issues from the review of PR #36
Two of these stopped the workflow working at all. The zip uploaded to Drive held a member called documentation-android.db, because `zip -j` drops the directory but keeps the basename and the staging file was named after this doc set. Every reader of that artifact looks for documentation.db exactly: this workflow's own download step, the daily docdb-regression-test run, and build-kotlin-docs. One live run would have broken all three until someone re-uploaded by hand. The local workflow already renamed before writing; the hosted one now does too. And "Run the unit tests" ran pytest without anything installing it. setup-python provides a clean interpreter, so the runner image's own packages are not on that path, and the job would have died at step 6 of 14 -- before converting a single page. The rest: - load_android_json_db.py checks up front that the database has every table it writes. Two of them, DocumentationDatabaseVersion and LastChange, are touched only by record_version at the very end, and nothing in this repository creates either, so a database without them lost the whole load -- 90 seconds locally, a quarter of an hour in CI -- to a "no such table" at the finish. Same fail-fast the media optimizer does for its content types. record_version and the preflight both have tests now; record_version had none at all, its two tables being absent from the fixture. - The Slack baton is a pair again. "Grabbing baton" fired unconditionally while "Dropping baton" was gated on dry_run being false -- and dry_run defaults to true, so the ordinary dispatch took a baton nothing ever dropped. Both halves are now gated on dry_run, and the release keys off the acquire's own conclusion under always(), so an early failure cannot strand it either. - The WIF token asks for drive.readonly as well as drive.file. drive.file covers only files the app itself created, and the step after it downloads a database it did not. docdb-regression-test.yaml already asks for both against the same file. - build-android-docs and build-kotlin-docs share one concurrency group, `documentation-db`. Both do download-modify-upload against the same Drive file, and per-workflow groups let them interleave, so whichever uploaded second silently discarded the other's work. This is the one change here outside the Android tree, and it is one line: a group only serialises runs that name it. - The red-link legend moved into a pageFooter() macro that all three templates call. reference() colours links on every page kind, but only class.peb explained what red meant -- a package summary or an index showed the colour with no legend. Verified by rendering one of each and grepping. - A test pins _MEMBER_KINDS, _MEMBER_ORDER and _PLURALS against each other. describe_tally silently drops a kind missing from the order while still reporting it as data, and count_label raises KeyError for one missing from the plurals, after the whole corpus has been converted. - HtmlLinks.java and rewrite_document said they mirrored each other. They do not: the Python side also folds each path to the case the database spells it with, for the StrictMode/strictmode pages. Both comments now say what each does and why they differ. - Dropped the "raw" key from the loader's stats, which was initialised and never touched again. Not fixed here: build-kotlin-docs.yaml has the same baton asymmetry and the same drive.file-only scope. Those are that workflow's to fix and are already addressed on fix/ADFA-4737; the concurrency group above is the only thing this PR needed from it. Tests: 164 Python (12 new) and 9 Java, all passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 7fe22be commit 0ae218f

12 files changed

Lines changed: 219 additions & 32 deletions

File tree

‎.github/workflows/build-android-docs-local.yaml‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,10 @@ jobs:
176176
- name: Install Python dependencies
177177
run: |
178178
pip install -r requirements.txt
179+
# pytest: the "Run the unit tests" step below needs it, and nothing else installs it.
180+
# setup-python gives a clean interpreter, so the runner image's own packages are not
181+
# on this path - without this line that step dies on `No module named pytest`.
182+
pip install pytest
179183
180184
- name: Build the Pebble renderer
181185
run: |
@@ -197,7 +201,13 @@ jobs:
197201
sqlite3 documentation.db "SELECT 1;" > /dev/null
198202
echo "DB_SIZE=$(stat -c%s documentation.db 2>/dev/null || stat -f%z documentation.db)" >> "$GITHUB_ENV"
199203
204+
# dry_run-gated, and paired with the release below. The baton stands for the lock on the
205+
# shared file this workflow rewrites, so a dry run - which rewrites nothing - should never
206+
# take it. Before this was gated, the default dispatch (dry_run is true) grabbed a baton
207+
# that the release, gated on dry_run being false, then never dropped.
200208
- name: 'Notify Slack: build started'
209+
id: slack_baton_taken
210+
if: ${{ !inputs.dry_run }}
201211
env:
202212
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}
203213
run: |
@@ -303,8 +313,14 @@ jobs:
303313
cp "$STAGED_DB" "$DB_PATH"
304314
echo "Wrote updated documentation.db back to $DB_PATH"
305315
316+
# always(), keyed off whether the acquire actually ran rather than off dry_run again.
317+
# Everything from the checkout to the Drive download can fail before "build started" is
318+
# reached, and a release gated only on dry_run announces a drop for a baton nobody took;
319+
# without always(), a failure after it leaves the baton held. The acquire's own conclusion
320+
# answers both - 'skipped' on a dry run and on an early failure, 'success' exactly when
321+
# Slack was told the baton had been taken.
306322
- name: 'Notify Slack: build complete'
307-
if: ${{ !inputs.dry_run }}
323+
if: ${{ always() && steps.slack_baton_taken.conclusion == 'success' }}
308324
env:
309325
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}
310326
run: |

‎.github/workflows/build-android-docs.yaml‎

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,12 @@ permissions:
5757
contents: read
5858
id-token: write
5959

60-
# This workflow overwrites a single shared Drive file - never let two runs
61-
# race to upload against each other.
60+
# Every workflow that rewrites the shared documentation.db on Drive shares this group, not just
61+
# the two runs of this one. Each does download -> modify -> upload against the same file, so an
62+
# Android run overlapping a Kotlin run means both start from the same revision and whichever
63+
# uploads second silently discards the other's work. A per-workflow group never saw that.
6264
concurrency:
63-
group: build-android-docs
65+
group: documentation-db
6466
cancel-in-progress: false
6567

6668
on:
@@ -175,6 +177,10 @@ jobs:
175177
- name: Install Python dependencies
176178
run: |
177179
pip install -r requirements.txt
180+
# pytest: the "Run the unit tests" step below needs it, and nothing else installs it.
181+
# setup-python gives a clean interpreter, so the runner image's own packages are not
182+
# on this path - without this line that step dies on `No module named pytest`.
183+
pip install pytest
178184
# google-api-python-client & friends: Drive download/upload, same
179185
# libraries check-tools/download_database.py already depends on.
180186
pip install google-api-python-client google-auth-httplib2 google-auth-oauthlib
@@ -198,7 +204,11 @@ jobs:
198204
with:
199205
workload_identity_provider: ${{ secrets.GCP_WIF_PROVIDER }}
200206
service_account: ${{ secrets.GCP_WIF_SERVICE_ACCOUNT }}
207+
# drive.readonly as well as drive.file: drive.file covers only files this app
208+
# created or opened itself, and the step below downloads a database it did not.
209+
# The same pair docdb-regression-test.yaml uses against the same file.
201210
access_token_scopes: |
211+
https://www.googleapis.com/auth/drive.readonly
202212
https://www.googleapis.com/auth/drive.file
203213
204214
- name: Download current documentation.db from Google Drive
@@ -214,7 +224,13 @@ jobs:
214224
rm -f documentation.zip
215225
echo "DB_SIZE=$(stat -c%s documentation.db 2>/dev/null || stat -f%z documentation.db)" >> "$GITHUB_ENV"
216226
227+
# dry_run-gated, and paired with the release below. The baton stands for the lock on the
228+
# shared file this workflow rewrites, so a dry run - which rewrites nothing - should never
229+
# take it. Before this was gated, the default dispatch (dry_run is true) grabbed a baton
230+
# that the release, gated on dry_run being false, then never dropped.
217231
- name: 'Notify Slack: build started'
232+
id: slack_baton_taken
233+
if: ${{ !inputs.dry_run }}
218234
env:
219235
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}
220236
run: |
@@ -318,7 +334,14 @@ jobs:
318334

319335
- name: Zip updated database for upload
320336
if: ${{ !inputs.dry_run }}
321-
run: zip -j documentation.zip "$STAGED_DB"
337+
run: |
338+
# The member inside the zip has to be called documentation.db, not whatever this
339+
# workflow happened to name its staging file: every reader of the Drive artifact looks
340+
# for that exact name and fails outright without it - this workflow's own download step
341+
# above, docdb-regression-test.yaml's daily run, and build-kotlin-docs.yaml. `zip -j`
342+
# drops the directory but keeps the basename, so the rename has to happen first.
343+
cp "$STAGED_DB" documentation.db
344+
zip -j documentation.zip documentation.db
322345
323346
- name: Upload updated database to Google Drive
324347
if: ${{ !inputs.dry_run }}
@@ -339,8 +362,14 @@ jobs:
339362
print(f"Uploaded new revision of {file_id}: {updated}")
340363
PYEOF
341364
365+
# always(), keyed off whether the acquire actually ran rather than off dry_run again.
366+
# Everything from the checkout to the Drive download can fail before "build started" is
367+
# reached, and a release gated only on dry_run announces a drop for a baton nobody took;
368+
# without always(), a failure after it leaves the baton held. The acquire's own conclusion
369+
# answers both - 'skipped' on a dry run and on an early failure, 'success' exactly when
370+
# Slack was told the baton had been taken.
342371
- name: 'Notify Slack: build complete'
343-
if: ${{ !inputs.dry_run }}
372+
if: ${{ always() && steps.slack_baton_taken.conclusion == 'success' }}
344373
env:
345374
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }}
346375
run: |

‎.github/workflows/build-kotlin-docs.yaml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,10 @@ permissions:
4747

4848
# This workflow overwrites a single shared Drive file - never let two runs
4949
# race to upload against each other.
50+
# Shared with build-android-docs.yaml: both rewrite the same documentation.db on Drive, so they
51+
# have to serialise against each other, not merely against a second run of themselves.
5052
concurrency:
51-
group: build-kotlin-docs
53+
group: documentation-db
5254
cancel-in-progress: false
5355

5456
on:

‎ProcessDocs/AndroidDocs/AndroidDocsToJson/load_android_json_db.py‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,12 @@ def rewrite_link(url: str, page_dir: str, known: dict) -> str:
124124

125125

126126
def rewrite_document(node, page_dir: str, known: dict):
127-
"""Every link in a page document, rewritten. Mirrors HtmlLinks.java, which does this for the
128-
standalone renderer; the two exist separately only because one is Java and one is Python."""
127+
"""Every link in a page document, rewritten.
128+
129+
HtmlLinks.java walks the document the same way for the standalone renderer, but the two are
130+
not interchangeable: `rewrite_link` below also canonicalises each path's case against what
131+
this database holds, which the renderer has no need of. See that class's own header.
132+
"""
129133
if isinstance(node, dict):
130134
result = {}
131135
for key, value in node.items():
@@ -218,7 +222,7 @@ def prepare(job: tuple, known: dict, compressor: DictionaryCompressor) -> tuple:
218222

219223

220224
def load(json_dir: Path, conn, workers: int, limit: int | None) -> dict:
221-
stats = {"updated": 0, "inserted": 0, "skipped": 0, "raw": 0, "stored": 0}
225+
stats = {"updated": 0, "inserted": 0, "skipped": 0, "stored": 0}
222226
language_id = get_id(conn, "Languages", "en-US")
223227
content_type_id, compress = get_content_type(conn, CONTENT_TYPE)
224228
if not compress:
@@ -267,6 +271,21 @@ def load(json_dir: Path, conn, workers: int, limit: int | None) -> dict:
267271
return stats
268272

269273

274+
# Every table this script reads or writes. Checked up front (see check_schema) because two of
275+
# them are only touched by record_version, at the very end: a database missing them would
276+
# otherwise take the whole load - 90 seconds locally, a quarter of an hour in CI - before saying
277+
# so. The same fail-fast the media optimizer does for its own content types.
278+
REQUIRED_TABLES = ("Content", "Languages", "ContentTypes", "Templates", "CompressionDictionary",
279+
"DocumentationDatabaseVersion", "LastChange")
280+
281+
282+
def check_schema(conn) -> list[str]:
283+
"""The tables in REQUIRED_TABLES this database does not have, in the order listed."""
284+
present = {name for (name,) in conn.execute(
285+
"SELECT name FROM sqlite_master WHERE type = 'table'")}
286+
return [name for name in REQUIRED_TABLES if name not in present]
287+
288+
270289
def record_version(conn, stats: dict) -> None:
271290
"""A row saying what changed, as the schema's own comment asks for. Minor, not major: nothing
272291
about the schema or the server's contract changes, a documentation set is re-expressed."""
@@ -306,6 +325,15 @@ def main(argv: list[str] | None = None) -> int:
306325
if args.out.resolve() == args.source.resolve():
307326
parser.error("--out must not be --source: the source database is never written to")
308327

328+
source_conn = sqlite3.connect(f"file:{args.source}?mode=ro", uri=True)
329+
try:
330+
missing = check_schema(source_conn)
331+
finally:
332+
source_conn.close()
333+
if missing:
334+
parser.error(f"{args.source} is missing table(s) this loader writes: "
335+
+ ", ".join(missing))
336+
309337
print(f"==> copying {args.source} -> {args.out}")
310338
shutil.copy2(args.source, args.out)
311339

‎ProcessDocs/AndroidDocs/AndroidDocsToJson/renderer/src/main/java/org/appdevforall/docs/android/HtmlLinks.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,14 @@
1717
*
1818
* It happens here rather than in a Pebble filter so that the templates need no filter of their
1919
* own: the database stores one self-contained template per row and evaluates it against the
20-
* page's JSON, with no opportunity to register anything. The same swap is implemented in
21-
* `load_android_json_db.py` for the same reason.
20+
* page's JSON, with no opportunity to register anything.
21+
*
22+
* `load_android_json_db.py`'s `rewrite_link` does the corresponding job for the database, and is
23+
* deliberately NOT identical: it also folds each path to the case the database spells it with,
24+
* because the database holds pages the scrape spelled two ways (`StrictMode/` and `strictmode/`)
25+
* and a link has to name the row that exists. There is no such second spelling here - this
26+
* renderer writes the HTML tree itself, mirroring the JSON tree's own paths - so the swap below
27+
* is the whole job. Change one and the other does not automatically need the same change.
2228
*/
2329
final class HtmlLinks {
2430

‎ProcessDocs/AndroidDocs/AndroidDocsToJson/renderer/src/main/resources/templates/_macros.peb‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,3 +201,20 @@
201201
{% endif %}
202202
</div>
203203
{% endmacro %}
204+
205+
{#
206+
The page footer, including the legend for the red links.
207+
208+
A macro rather than three copies because the colouring is applied by `reference` above, which
209+
every page kind uses: a class page, a package summary and an index all render red links, so all
210+
three need the sentence that says what red means. It lived on class.peb alone, and a reader
211+
landing on a package summary saw the colour with nothing to explain it.
212+
#}
213+
{% macro pageFooter() %}
214+
<footer>
215+
<p>Generated from the developer.android.com reference scrape. A link shown in red is not part of
216+
this documentation: nearly all of them leave for the web and need a network connection -- most
217+
back to developer.android.com, the rest to other sites the reference cites -- and a handful name
218+
nothing at all, because the <code>href</code> in the source is malformed.</p>
219+
</footer>
220+
{% endmacro %}

‎ProcessDocs/AndroidDocs/AndroidDocsToJson/renderer/src/main/resources/templates/class.peb‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -103,11 +103,6 @@
103103
{% endfor %}
104104

105105
</main>
106-
<footer>
107-
<p>Generated from the developer.android.com reference scrape. A link shown in red is not part of
108-
this documentation: nearly all of them leave for the web and need a network connection -- most
109-
back to developer.android.com, the rest to other sites the reference cites -- and a handful name
110-
nothing at all, because the <code>href</code> in the source is malformed.</p>
111-
</footer>
106+
{{ pageFooter() }}
112107
</body>
113108
</html>

‎ProcessDocs/AndroidDocs/AndroidDocsToJson/renderer/src/main/resources/templates/index.peb‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,6 @@
5050
{% endfor %}
5151

5252
</main>
53-
<footer>
54-
<p>Generated from the developer.android.com reference scrape.</p>
55-
</footer>
53+
{{ pageFooter() }}
5654
</body>
5755
</html>

‎ProcessDocs/AndroidDocs/AndroidDocsToJson/renderer/src/main/resources/templates/package.peb‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,6 @@
4646
{% endfor %}
4747

4848
</main>
49-
<footer>
50-
<p>Generated from the developer.android.com reference scrape.</p>
51-
</footer>
49+
{{ pageFooter() }}
5250
</body>
5351
</html>

‎ProcessDocs/AndroidDocs/AndroidDocsToJson/test_android_docs_to_json.py‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1102,3 +1102,34 @@ def test_a_class_the_source_already_set_is_kept(self, linker, tmp_path):
11021102
"""
11031103
page = parse(markup, "android/demo/Widget.html", linker, tmp_path)
11041104
assert 'class="api-reference external-link"' in page["description"]
1105+
1106+
1107+
class TestKindTablesAgree:
1108+
"""Three tables have to name the same kinds, and none of them says so.
1109+
1110+
A kind is counted by `_MEMBER_KINDS`, described only if it is in `_MEMBER_ORDER`, and
1111+
labelled only if it is in `_PLURALS`. `describe_tally` iterates the order and skips anything
1112+
not in it, so a kind missing there is dropped from every package's prose while still riding
1113+
on the row as data; `count_label` subscripts `_PLURALS`, so a kind missing there raises
1114+
KeyError after the whole corpus has been converted.
1115+
"""
1116+
1117+
def test_every_counted_member_kind_is_described(self):
1118+
counted = set(extractor._MEMBER_KINDS.values()) | {"nested type"}
1119+
assert counted == set(extractor._MEMBER_ORDER)
1120+
1121+
def test_every_described_kind_has_a_plural(self):
1122+
assert set(extractor._MEMBER_ORDER) <= set(extractor._PLURALS)
1123+
assert set(extractor._TYPE_ORDER) <= set(extractor._PLURALS)
1124+
1125+
def test_every_kind_class_kind_returns_is_described(self):
1126+
# package_tally falls back to "class" for a page whose kind could not be read, so that
1127+
# one has to be in the order too.
1128+
produced = {"annotation", "enum", "object", "record", "interface", "class"}
1129+
assert produced == set(extractor._TYPE_ORDER)
1130+
1131+
def test_count_label_covers_every_kind(self):
1132+
# The subscript in count_label is the crash; this is the cheapest proof it cannot fire.
1133+
for kind in (*extractor._TYPE_ORDER, *extractor._MEMBER_ORDER):
1134+
assert extractor.count_label(2, kind).startswith("2 ")
1135+

0 commit comments

Comments
 (0)