Skip to content

[core] Write custom option headers to an env independent include dir - #400

Open
bdraco wants to merge 3 commits into
libretiny-eu:masterfrom
bdraco:custom-options-stable-include
Open

[core] Write custom option headers to an env independent include dir#400
bdraco wants to merge 3 commits into
libretiny-eu:masterfrom
bdraco:custom-options-stable-include

Conversation

@bdraco

@bdraco bdraco commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Follow up to esphome/esphome#17726, which enabled ccache for ESPHome's LibreTiny builds; sharing the cache between devices is blocked by this generated header path, so the remaining fix belongs here.

The generated custom option headers (lwipopts.h, sys_config.h and friends) are currently written to ${BUILD_DIR}/include, a path that embeds the PlatformIO env name. That -I flag makes every compile command unique per env, so compiler caches like ccache can never share results between projects that differ only by env name; ESPHome creates one env per device, so nothing is shared between devices.

This writes them to ${PROJECT_BUILD_DIR}/include/ instead, where the hash is derived from the option content. The path no longer contains the env name, so with ccache base_dir the compile commands hash identically across devices with the same options; envs with different options get different directories, so they can never pick up each other's generated headers, and concurrent builds of envs with identical options write identical bytes. The header writes also moved to a with block so a failed build cannot leave a truncated header behind.

Verified with ESPHome pointing the framework source at this branch; a clean bk72xx build succeeds, the headers land in the content keyed directory, 292 dependency files reference them at the new path with none referencing the old one, and a second compile reproduces the same hash with nothing rebuilt.

@esphbot

esphbot commented Jul 27, 2026

Copy link
Copy Markdown

Previous review — superseded by a newer review below.

@esphbot esphbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Important issues found.

  • Shared include dir leaks generated headers between envs of the same project

@esphbot

esphbot commented Jul 27, 2026

Copy link
Copy Markdown

Previous review — superseded by a newer review below.

@esphbot esphbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@esphbot

esphbot commented Jul 28, 2026

Copy link
Copy Markdown

PR Review — [core] Write custom option headers to an env independent include dir

Both prior suggestions are cleanly addressed; nothing blocking remains. Approve.

What's solid in this revision:

  • The write path is now genuinely race-free. Content is built in memory, compared against the existing file, and only written via <header>.<pid>.tmp + os.replace() — atomic on POSIX and Windows. That closes the truncate-then-rewrite window two concurrent envs sharing a hash dir could hit, and it also removes the half-written-header-survives-a-crash case that the old bare open(..., "w") had.
  • The skip-if-unchanged compare is correct across platforms: text-mode write translates \n\r\n on Windows and text-mode read translates it back, so the comparison round-trips and never degenerates into a rewrite every build.
  • Content-compare (rather than just isfile) is the right check, not redundancy: if a future LibreTiny release changes the generated header format, the hash key is unchanged (it is derived from the options only) but the stale file is still detected and replaced.
  • The accumulation note from the prior review is now in the comment block, with the reason — pruning siblings would race a concurrent build of another env. That is the right trade and future readers won't mistake it for an oversight.
  • Hash input is still deterministic: env_parse_custom_options (builder/utils/env.py:142-158) normalises each block to a plain str -> str dict before env_apply_custom_options runs (builder/frameworks/base.py:18 precedes builder/utils/libs-queue.py:188), and RecursiveDict is a dict subclass (tools/libretiny/dict.py:6) that platform.py:101 already round-trips through json.dumps.
  • Diff still matches the description exactly — no scope creep, and docs/dev/config.md documents only the custom_options syntax, not the output path, so no docs update is owed.

No new issues found. Residual rough edges are inert and not worth changing: a .tmp file left behind by a failed write is never included by name, and the directories are only reachable via CPPPATH for the env that just generated them.


✅ Resolved since last review (1)

Previously-flagged issues verified fixed
  • builder/utils/env.py:182 Shared header file is rewritten non-atomically, so concurrent envs with identical options can race


Checklist

  • Change matches PR description, no scope creep
  • Build isolation preserved across envs (content-keyed dir)
  • Hash input deterministic and JSON-serializable
  • Concurrent-build safety of generated artifacts (prior suggestion applied)
  • Resource management — files opened via with
  • Stale/leftover artifact handling documented (prior suggestion applied)
  • PlatformIO variable exists and substitutes correctly
  • No secrets, injection, or unsafe path handling
  • Documentation stays in sync

Automated review by Kōan (Claude) HEAD=12b5b34 2 min 5s

@esphbot esphbot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bdraco
bdraco marked this pull request as ready for review July 28, 2026 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants