Fix use of vendored json when external is chosen#4782
Open
gdt wants to merge 4 commits into
Open
Conversation
Contributor
Author
|
On my system, with normal-system-dependency nlohmann/json, this branch builds and passes tests as well as master does (one issue in test_network already reported on the list), providing confidence that now all compilations using the json implementation switcher have the flag passed. |
The rest of the CI builds use the vendored copy. For Fedora, install the external version so that those codepaths will be tested.
This fixes using the outdated vendored copy when external was configured.
Two test programs use json and thus need the define to avoid misusing the vendored version, when the plan is external.
8f28ace to
980e84d
Compare
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.
nlohmann/json can either be a normal dependency ("external", the standard approach) or a vendored copy (for those who don't have it installed). There is a header that switches on
EXTERNAL_NLOHMANN_JSONto use the system version instead of the vendored version. The build system fails to pass it to some compilations that use json.This PR, minus the first commit which serves as a test, adds those defines.
An alternative change in the same spirit would be to add the define to all builds, which would be more robust than adding it to builds that use json today. A further change that would improve robustness would be to flip the define to VENDORED_NLOHMANN_JSON and thus if there are new targets that use json with passing it, builds in environments lacking system nlohmann/json will fail, instead of silently using the old code.
It will be interesting to see how CI goes for this branch, which has (temporarily for testing) removed the vendored code. The plan would be to rebase to drop that commit before merging.