Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dspreitz
force-pushed
the
ci/run-content-checks
branch
2 times, most recently
from
September 22, 2026 22:16
e95d3ae to
758a4b0
Compare
Contributor
Author
|
The red This wires Merge order: #567, then this. With #567 in, the same run reports One thing still open here. The waypoint check reports rather than fails, and by the reasoning in #561 it should fail. It cannot yet: |
dspreitz
force-pushed
the
ci/run-content-checks
branch
2 times, most recently
from
September 23, 2026 05:41
461b948 to
409f865
Compare
Contributor
Author
|
The waypoint check now gates, in aerofiles 1.5.6 was released this morning and no longer discards a whole CUP file over an observation zone key it does not model — that was what rejected Swept all 84 waypoint files with 1.5.6:
Every rejection that remains is a real defect, so the check can gate without blocking anything legitimate. Verified both directions: against The Merge order for this PR — it now depends on four things rather than one, all of them open and green:
Without #572 the waypoint check fails on two good files again; without #566/#568/#569 it fails on four files that are genuinely defective today. |
aerofiles 1.5.5 imports dateutil but declares no dependencies of its own, so pip install -r requirements.txt does not bring it in. Every script that parses OpenAir has been working only because the GitHub runner image happens to ship python-dateutil; the same commands fail on a clean checkout.
check_airspaces.py could never have run: aerofiles' Reader takes the file as a required constructor argument, so the script raised TypeError before opening anything, whatever it was passed. check.sh also hands it a directory rather than a file, and check.sh itself is wired into no workflow, so nothing has been checking airspace content. Take files or directories, and report per file instead of throwing on the first problem. Read bytes and fall back to latin-1, because the published set is not all UTF-8: NL-ASP-National-XCSoar.txt is ISO-8859-1 and cannot be decoded as UTF-8 at all. The decision of what counts as airspace moves to script/lib so that the URI check can reach the same verdict. One parsed airspace is the minimum, because a single-area file is legitimate -- DE-ASP-Military-Low-Flying.txt holds exactly one -- while anything serving an error page parses as none.
A HEAD request only proves that something answered. An airspace URI that starts serving an error page still passes it, and the file reaches the pilot describing nothing: daec.de answers a missing file with HTTP 200 and a 54 byte error body rather than a 404, and aip.net.nz answers 200 with an Incapsula block page. Entries of type airspace pointing at a .txt are now downloaded and parsed; everything else keeps the cheap HEAD. Knowing an entry's type means reading whole manifest records rather than bare uri= lines, so the manifest is parsed into records. Hosts that refuse the client are reported apart from failures. gliding.co.nz answers 403 to every path including / and /robots.txt from a data centre, and a Cloudflare challenge to anything without a browser, so a runner cannot tell a blocked file from a removed one. Failing on that would leave the check permanently red over a file pilots download without trouble, so 403, 429 and 451 are listed separately and 404 and 500 still fail. Also accept a local path, which is how check.sh has always invoked this and which raised MissingSchema every time: get_urls_from_file() existed but was never called.
A bare pass or fail does not say what to look at. Counting "AC" lines
without parsing gives the size of the file, so the log can contrast it with
what aerofiles actually reached: "4 airspace blocks, aerofiles read 0
(10 parse errors, no bbox)" names the file, the scale of the problem, and
the consequence. Clean files stay on the short form, "288 airspaces".
The verdict itself stays with aerofiles. XCSoar accepts more than the format
allows -- a lowercase hemisphere, and degrees/minutes/seconds beside decimal
minutes, which the specification forbids outright ("Do not mix DMS and DDM
notations. Stick to one definition throughout the file.") -- and passing a
file because XCSoar copes would hide the defect rather than get it fixed.
Review on XCSoar#561: the point of a quality check is to get people to fix their data, and XCSoar being tolerant is not a reason to be. That holds without reservation for the files in data/content, which are ours to repair. A single damaged record costs the airspace it belongs to and costs the whole file its bbox, since repository.py derives that from the same parser, so tolerating one buys nothing. check_airspaces.py now requires a clean parse. It does not hold for files reached over a URI. A third-party source with a damaged record cannot be fixed here, and failing the run over it would leave the check permanently red on somebody else's file; BR, CA and JP each carry such records today. check_urls.py therefore keeps the lenient test, which still catches the case that matters there -- a URI that has started serving an error page instead of airspace. The two policies are named rather than implied: is_clean_openair() for what we ship, looks_like_openair() for what we link to. Every file in data/content passes the strict test once XCSoar#567 lands; before it, CA-ASP-National, MW-ASP-National and DE-ASP-HahnweideComp2017 fail it, which is the point.
iter_airspace_files() passed over a directory with no *.txt, so a run that was given a readable file alongside such a directory reported success without ever saying it had checked nothing from it. That matters because check.sh hands this the whole airspace tree. A build that stopped producing airspace, or an output layout that moved, would look exactly like a clean run. Only the case of no arguments producing nothing at all was caught, and that is not the case that will happen. Empty directories are now reported per argument and fail the run: FAIL no *.txt in directory output/content/airspace Reported by CodeRabbit on XCSoar#561. The function had no tests; it has six now, covering nested expansion, an empty directory beside a good file, a bare file argument, and no arguments at all.
Both checks had tests for how they read their input and none for what they do with a verdict once they have one. check_airspaces.py: every MainTest case exercised the argument expansion, so main() could have collected check_file() failures and then dropped them while the file stayed green. One malformed file now goes through main() and its exit status is asserted -- the body is the TYPO3 error text the DAeC server really served in place of airspace. check_urls.py: nothing called check_urls() at all. looks_like_openair() was tested directly in test_openair_content.py, but not the path that reaches it, so a regression that skipped the content check, or ran it and disregarded the answer, would have reported an HTML error page as PASS with every test passing. The new case answers 200 with a 404 page and asserts the URL is reported failed and not merely blocked; its counterpart answers 200 with real airspace, without which a check that failed everything would satisfy the first. Verified by breaking each path on purpose: dropping the failure from main() fails only the first new test, and skipping looks_like_openair() in check_record() fails only the second. 36 tests pass.
The file this repository ships has to parse without a single error, which is a stricter promise than "holds some airspace", and it is the reason check_airspaces.py calls is_clean_openair() where check_urls.py calls looks_like_openair(). Nothing held it to that: the failure cases went through main() with bodies carrying no airspace at all, which the lenient test rejects just as readily. Swapping check_file() to looks_like_openair() therefore left all 36 tests passing. A file that satisfies the lenient test and fails the strict one closes that: real airspace followed by a record mixing coordinate notations, the defect this check was written to catch. With it, that swap fails exactly one test.
check.sh has never run in CI, which is how a crashing airspace check and several unreadable files survived unnoticed. Run it after build.sh, before the deploy step, so a bad build cannot reach download.xcsoar.org. Fix the path contract while wiring it up. check.sh defaulted its output directory to ./output/content and then looked for the manifest at ./output/content/repository, but build.sh writes content to ./output/content and the manifest to ./output/repository, so no single value could satisfy both. It now takes the build root, matching build.sh. Add --offline, used by the PR job. The URI check downloads and parses every airspace file the manifest points at, tens of megabytes across a few dozen third-party hosts; that is worth doing daily, which check_repo_urls.yml already does against the published manifest, but not on every push where an unrelated host being down would block merging. Waypoint parsing reports instead of failing, for now. Three files in the published set do not parse -- GLB-WPT-ProvingGrounds-XCSoar.cup and CZ-WPT-WaveCamp-2022-OBv1.cup on "Reading frequency failed", ZA_Cape_2023-11-10.cup on "Reading elevation failed". aerofiles has already proved stricter than XCSoar's own parser on airspace in this repository, so those failures do not establish that a pilot cannot read the files, and gating on them would block every PR over a question nobody has answered yet. They are printed as WARNING lines so they stay visible.
The waypoint check reported instead of failing because two files it rejected were not actually broken: CZ-WPT-WaveCamp-2022-OBv1.cup and GLB-WPT-ProvingGrounds-XCSoar.cup carry SpeedStyle and MaxAlt in their task sections, and aerofiles 1.5.5 discarded the whole file over an observation zone key it did not model. Gating on that would have blocked two files with nothing wrong with them. aerofiles 1.5.6 fixes it, and the pin moves in a separate change. Sweeping all 84 waypoint files with 1.5.6: four are rejected against master today -- the two above plus ZA_Cape_2023-11-10.cup and FR-WPT-National-XCSoar.cup -- and none once the repairs in XCSoar#566, XCSoar#568 and XCSoar#569 are in. Every rejection left is a real defect, so the check can gate. Verified both directions: against master's data the run exits 1 and names four files; with the repairs applied it exits 0. The WARNED path goes with it, having no remaining use.
dspreitz
force-pushed
the
ci/run-content-checks
branch
from
September 26, 2026 07:07
c8855e4 to
6bb06bb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #561, which repairs the checks this runs. Draft until that merges; the diff here will then be the last commit only.