Strip folly's own root from XLOG categories, not the outer project's - #2677
Open
gmarzot wants to merge 1 commit into
Open
Strip folly's own root from XLOG categories, not the outer project's#2677gmarzot wants to merge 1 commit into
gmarzot wants to merge 1 commit into
Conversation
The comment above this property says the intent: derive log category names from "the relative portion of the source file name inside the folly repository". But the value uses CMAKE_SOURCE_DIR / CMAKE_BINARY_DIR, which point at the top-level project. Those equal folly's own roots only when folly is the top-level build. When folly is embedded — FetchContent, add_subdirectory, or any superproject — CMAKE_SOURCE_DIR is the consumer's root and is not a prefix of folly's source paths, so xlogStripFilename() strips nothing and every category falls back to the full absolute __FILE__. Selectors like --logging=folly=DBG2 then match nothing, and category names leak absolute build paths (e.g. home.runner.work...). Use CMAKE_CURRENT_SOURCE_DIR / CMAKE_CURRENT_BINARY_DIR, which always resolve to folly's own roots here — the same variable the surrounding set_property() call already uses to scope the directory. No behavior change when folly is top-level, since the two are equal in that case.
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
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.
Problem
The comment above this property states the intent:
but the value is built from
CMAKE_SOURCE_DIR/CMAKE_BINARY_DIR, which point at the top-level project. Those equal folly's own roots only when folly is the top-level build.When folly is embedded — FetchContent,
add_subdirectory, or any superproject —CMAKE_SOURCE_DIRis the consumer's root and is not a prefix of folly's source paths.xlogStripFilename()then strips nothing, and every folly category falls back to the full absolute__FILE__. Two consequences for embedded consumers:--logging=folly=DBG2and everyfolly.*selector match nothing; only the root level works.home.runner.work.myproject._deps.folly-src.folly.io.async.AsyncSocket.Fix
Use
CMAKE_CURRENT_SOURCE_DIR/CMAKE_CURRENT_BINARY_DIR. Evaluated in folly's own top-levelCMakeLists.txt, these always resolve to folly's source and build roots regardless of how folly is embedded — and it's the same variable the surroundingset_property(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} …)call already uses to scope the property.No behavior change when folly is the top-level project, since
CMAKE_SOURCE_DIR == CMAKE_CURRENT_SOURCE_DIRin that case. The change only affects builds where the current behavior is already broken.Test plan
__FILE__and--logging=folly=DBG2selected nothing; after, categories derive asfolly.*and the selector scopes correctly.FOLLY_XLOG_STRIP_PREFIXESexpands identically before and after.Same class of fix as facebookexperimental/moxygen#207, which corrected the identical
CMAKE_SOURCE_DIRassumption in moxygen's own use of this define.