Skip to content

discussion: distcheck's Platform.exe mirrors a field packages/ts/src/platforms.ts has never had #1004

Closed

Description

@thekevinbot

Discussion, not a work item — please don't open a PR off this. The mechanical change is three lines; what's worth deciding first is what "mirror" is supposed to mean for this dataclass, because the answer decides whether exe gets deleted, gets a different justification, or gets joined by two fields it's currently missing.

Found while fixing #996 (PR #999), in the module next door. Not folded into that PR — it's out of #996's stated scope, and a previous pass evidently looked straight at this field and chose to keep it, so overruling that silently inside an unrelated fix seemed wrong.

What's there

internals/distcheck/src/distcheck/node_flow/platforms.py:21 carries exe: bool on the Platform dataclass. Nothing in internals/distcheck reads it outside its own tests — three assertions, at platforms_test.py:30, :42, and :65.

Two of those tests state, in comments, why it's still there:

def test_find_host_platform_windows_addon_has_no_exe_suffix():
    # The `.exe` suffix belonged to the standalone binary, which is no longer
    # published (#739); an addon is a `.node` on every platform.
def test_only_windows_rows_flag_exe():
    # `exe` still marks the Windows row (it mirrors packages/ts/src/platforms.ts),
    # but nothing derives a filename from it now that no binary is published.

So the field is knowingly dead weight, kept on one stated ground: that it mirrors the TypeScript source of truth.

The problem with that ground

It doesn't hold. packages/ts/src/platforms.ts has no exe field, and going by git log -S, it never has:

$ git log --all -S'exe' -- packages/ts/src/platforms.ts
(no output)

Its Platform interface is triple, nodePlatform, nodeArch, libName, os, cpu, libc?. There is nothing on the TypeScript side for exe to mirror, which means the second comment's justification has been false since it was written.

For provenance: platforms.py was created whole in 6502217, on claude/739-npm-in-process — the #739 branch itself. So exe didn't survive #739, it was born on it, carried over from the pre-#739 world where a standalone binary really did need an .exe suffix on Windows. It arrived already vestigial.

Why this is a question and not just a deletion

The mirror is loose in both directions, which is the part I don't think should be settled by whoever happens to touch the file next:

platforms.ts platforms.py
triple ✅ ❌ missing
libc ✅ (Linux rows) ❌ missing
exe ❌ never existed ✅ present, unused

platforms.py's own docstring says it is "A Python port of the host-row lookup in packages/ts/src/platforms.ts" holding "enough of each published target to reconstruct the host's @dirsql/lib-<slug> sub-package", and PLATFORMS is commented "Keep in lockstep with that file". Those two framings pull opposite ways — "enough of" licenses the omissions, "lockstep" forbids them — and exe is what falls out of the gap.

So the question is which framing wins:

  1. "Enough of" — a deliberate subset. Then exe should go (it isn't needed to reconstruct anything), the missing triple / libc are correct as-is, and the "Keep in lockstep" comment is the thing that's misleading and should be reworded. Cheapest option, and my guess at the right one.
  2. True lockstep. Then exe still goes, but triple and libc should be added, and it's worth asking whether a check should enforce the correspondence rather than a comment asserting it — a drifting hand-maintained mirror is what produced both this and distcheck: node flow's @dirsql/cli-<slug> prerequisite left behind by #739 (integration test raises, stale gate docstring + README) #996. That's a real cost; the payoff is that a new published target can't be half-added.
  3. Keep exe. Fine by me if there's a reason I can't see — but the comment giving that reason needs to be replaced, because as written it points at a field that isn't there.

The reason I'd rather not just push (1) is that a mirror nobody checks is the shared root cause of #972, #996, and this. Deleting one dead field tidies the symptom and leaves the mechanism intact, and whether that mechanism is worth hardening is a maintainer call about cost, not a cleanup.

Not urgent

Nothing is broken. exe is inert — no filename, path, or package name derives from it, and the distcheck flows pass. This is a correctness-of-the-record issue, and the sharpest part is that a comment currently explains a decision by citing something untrue, which is exactly the kind of thing a future reader would take at face value.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions