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.

Sign up for GitHub

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

Jump to bottom

Fix build when JSON_USE_EXCEPTION == 0 #332

Merged
merged 2 commits into from
Mar 14, 2023
Merged

Fix build when JSON_USE_EXCEPTION == 0 #332

merged 2 commits into from
Mar 14, 2023

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Mar 13, 2023

🎉 New feature

Summary

Redo of #309.

Newer versions of jsoncpp do not provide exception classes when JSON_USE_EXCEPTION is off.

Updating the #if directives in JSONParse.cc allows this file to compile against jsoncpp configured to not throw.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Mar 13, 2023
@nkoenig nkoenig mentioned this pull request Mar 13, 2023
Closed
8 tasks
@azeey azeey requested a review from mjcarroll March 13, 2023 18:49
mjcarroll
mjcarroll reviewed Mar 13, 2023
View reviewed changes
Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

My preference here would be to define something in one place like

#define JSON_HAVE_EXCEPTIONS ...

and then use that to block off sections in the rest of the file. This would help future maintenance.

@nkoenig
Use central define
0ae3c14 
Signed-off-by: Nate Koenig <[email protected]>
@nkoenig
Copy link
Contributor Author

nkoenig commented Mar 14, 2023

Done in 0ae3c14

@nkoenig nkoenig requested a review from mjcarroll March 14, 2023 17:32
@nkoenig
Copy link
Contributor Author

nkoenig commented Mar 14, 2023

The failing tests are related to the fuel server download reliability.

@nkoenig nkoenig merged commit cb7030c into gz-fuel-tools8 Mar 14, 2023
@nkoenig nkoenig deleted the json_use_exception branch March 14, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

Assignees
No one assigned
Labels
🌱 garden Ignition Garden
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nkoenig @mjcarroll