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

Require JSON #1254

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Require JSON #1254

merged 4 commits into from
Jun 18, 2024

Conversation

sethrj
Copy link
Member

@sethrj sethrj commented May 27, 2024

Follow-on to #1253 : we're using JSON increasingly and will require it for the python front end (https://github.com/celeritas-project/celerpy/) and now have the ability to automatically provide it if it's not on the system. Requiring JSON simplifies a lot of the code, and other packages in the typical HEP pipeline (viz., ROOT) require nlohmann-json.

The only downside I see is the increased chance of causing a conflict in a multi-library toolchain (in which case nljson is hopefully already found, since e.g. ROOT is loaded before the initial decision to use JSON as an external?) or for G4VG where JSON isn't ever used in practice but this would require it.

@sethrj sethrj added core Software engineering infrastructure minor Minor internal changes or fixes labels May 27, 2024
@sethrj sethrj requested review from drbenmorgan and pcanal May 27, 2024 13:56
@sethrj sethrj marked this pull request as draft May 28, 2024 13:37
@sethrj sethrj force-pushed the require-json branch 2 times, most recently from f516168 to 7af7ba0 Compare May 29, 2024 20:18
@drbenmorgan
Copy link
Contributor

I think requiring nlohmann-json is fine for the reasons you've outlined. Noted that this is still in Draft and has some CI failures, so will approve once these are fixed.

@sethrj sethrj marked this pull request as ready for review June 18, 2024 16:53
Copy link
Contributor

@drbenmorgan drbenmorgan left a comment

Choose a reason for hiding this comment

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

Ci good, so LGTM @sethrj!

@sethrj sethrj enabled auto-merge (squash) June 18, 2024 19:22
@sethrj sethrj merged commit c5120a3 into celeritas-project:develop Jun 18, 2024
29 checks passed
@sethrj sethrj deleted the require-json branch July 23, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Software engineering infrastructure minor Minor internal changes or fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants