Skip to content

Stop rebuilding Proxygen StatsWrapper#6102

Closed
mavam wants to merge 1 commit into
mainfrom
topic/cmake-statswrapper-fix
Closed

Stop rebuilding Proxygen StatsWrapper#6102
mavam wants to merge 1 commit into
mainfrom
topic/cmake-statswrapper-fix

Conversation

@mavam
Copy link
Copy Markdown
Member

@mavam mavam commented Apr 30, 2026

🔍 Problem

  • The bundled Proxygen StatsWrapper.h workaround uses an always-dirty custom target.
  • Ninja regenerates the header on every incremental build, even when inputs did not change.

🛠️ Solution

  • Generate the build-tree copy during CMake configuration instead.
  • Keep the generated header present before Ninja evaluates Proxygen's generated-file rule.
  • Remove the custom target that forced repeated regeneration.

💬 Review

  • Focus on whether configure-time generation is the least invasive workaround for the vendored Proxygen rule.
  • Verified with repeated cmake --build --preset macos-xcode-release; the second build no longer regenerates StatsWrapper.h.

@github-actions github-actions Bot added the build CI/CD, packaging, and build system label Apr 30, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f2520902e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread CMakeLists.txt Outdated
@mavam mavam force-pushed the topic/cmake-statswrapper-fix branch from 8f25209 to 79ac661 Compare April 30, 2026 08:02
Generate the bundled Proxygen StatsWrapper header during CMake configuration instead of through an always-dirty custom target. This keeps the build-tree header present without making Ninja rerun the generator on every incremental build.

Assisted-by: GPT-5 (pi)
@mavam mavam force-pushed the topic/cmake-statswrapper-fix branch from 79ac661 to c562082 Compare April 30, 2026 08:10
Copy link
Copy Markdown
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

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

Please don't invest so much time into this workaround. The issue is upstream.

You can open a PR there to change the custom command to

add_custom_command(
    OUTPUT ${PROXYGEN_GENERATED_ROOT}/proxygen/lib/stats/StatsWrapper.h
    COMMAND
        ${CMAKE_COMMAND} -E make_directory
        ${PROXYGEN_GENERATED_ROOT}/proxygen/lib/stats
    COMMAND
        ${CMAKE_CURRENT_SOURCE_DIR}/stats/gen_StatsWrapper.sh
        ${PROXYGEN_GENERATED_ROOT}
    DEPENDS
        ${CMAKE_CURRENT_SOURCE_DIR}/stats/gen_StatsWrapper.sh
        ${CMAKE_CURRENT_SOURCE_DIR}/stats/BaseStats.h
    COMMENT "Generating StatsWrapper.h"
    VERBATIM
)

until that is available we can accept the few milliseconds the generator takes.
Once that is fixed we can drop the entire statswrapper workaround.
Edit: the code to change is in: https://github.com/facebook/proxygen/blob/main/proxygen/lib/CMakeLists.txt#L30

@mavam
Copy link
Copy Markdown
Member Author

mavam commented Apr 30, 2026

Closing this in favor of an upstream Proxygen fix: facebook/proxygen#614

The local workaround became too involved for the small incremental-build cost. Once upstream is fixed and the vendored Proxygen is updated, we can drop the existing StatsWrapper workaround entirely.

@mavam mavam closed this Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build CI/CD, packaging, and build system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants