Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move bundling of JSON-C to CMake. #17207

Merged
merged 6 commits into from Apr 10, 2024
Merged

Conversation

Ferroin
Copy link
Member

@Ferroin Ferroin commented Mar 20, 2024

Summary

Similar rationale to #17190, our handling of JSON-C suffers from the same potential discrepancy between how our installer code sees the build environment and how CMake sees it. Shifting JSON-C vendoring into CMake eliminates that issue, and also allows for slightly better parallelization of the build/install process.

Test Plan

We actually validate vendoring of JSON-C in CI, so testing should not require anything other than confirming CI passes on this PR.

@github-actions github-actions bot added area/packaging Packaging and operating systems support area/build Build system (autotools and cmake). labels Mar 20, 2024
@Ferroin
Copy link
Member Author

Ferroin commented Mar 22, 2024

Rebased to resolve merge conflicts and pick up latest changes.

@Ferroin Ferroin force-pushed the cmake-jsonc-vendoring branch 2 times, most recently from adea2a0 to 08a1274 Compare March 28, 2024 12:55
@Ferroin
Copy link
Member Author

Ferroin commented Mar 28, 2024

Rebased again to pick up latest changes in master.

@Ferroin Ferroin marked this pull request as ready for review April 3, 2024 17:03
@Ferroin Ferroin requested review from a team and vkalintiris as code owners April 3, 2024 17:03
Copy link
Contributor

@vkalintiris vkalintiris left a comment

Choose a reason for hiding this comment

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

If you address my comment (or disagree with it) feel free to red button this.

#
# The specified target must already exist, and the netdata_detect_json-c
# macro must have already been run at least once for this to work correctly.
function(netdata_add_jsonc_to_target _target)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, don't do this. Over time it will become difficult to figure out easily the dependency DAG of targets.

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m not sure how this significantly complicates things, unless you’re using some external script to parse the CMakeLists file and try to figure out the dependency graph from that.

This function being called on a target says quite clearly that that target depends on JSON-C. And because of how CMake handles adding dependencies, you need to be searching the whole file for the target name anyway instead of just blindly looking for target_* calls on the target. I’m also trying to follow a netdata_add_*_to_target pattern for functions like this so that they are easy to find and properly descriptive.

To clarify, my intent here is to keep all the arcana involved in getting this to actually work properly encapsulated. By handling it like this, anybody working on the build system won’t need to remember the names of the variables used to add JSON-C to a target, or where they are set, or whether the include directories need to be appended or prepended to the search list, or the fact that you need to add a supplementary dependency to the target to ensure that headers can be found correctly when using a vendored copy of JSON-C.

@Ferroin Ferroin merged commit 0f5b137 into netdata:master Apr 10, 2024
145 of 149 checks passed
@Ferroin Ferroin deleted the cmake-jsonc-vendoring branch April 10, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build system (autotools and cmake). area/packaging Packaging and operating systems support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants