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

Include jsonCPP headers via #include "json/json.h" #42516

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

Flamefire
Copy link
Contributor

Don't use #include "include/json/json.h" which is unusual and therefore confusing
This allows to remove the header symlinking done for the system lib version

Closes #42303

Disclaimer: I've tested the system build with this which works, not 100% sure about Bazel as I'm not very familiar with it but the includes = ["include"], in the cc_library rule should mean this works.

Don't use #include "include/json/json.h" which is unusual and therefore confusing
This allows to remove the header symlinking done for the system lib version

Closes tensorflow#42303
@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Aug 20, 2020
@gbaned gbaned self-assigned this Aug 20, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Aug 20, 2020
@perfinion perfinion added the kokoro:force-run Tests on submitted change label Aug 20, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 20, 2020
@perfinion
Copy link
Member

Thanks for doing this, the symlinking in jsoncpp systemlibs has annoyed me since the beginning. I made a PR some time ago ( #38327 ) but hit some test failures and was too busy to resolve the last ones.
I kicked off the tests on this one so lets see if it works better now.

Again thanks so much for taking the time to work on this!

@Flamefire
Copy link
Contributor Author

Seems like the build worked. Seeing some strange errors on OSX related to tf::DataType* or so which looks unrelated and LLVM ERROR: Building op std.return but it isn't registered in this MLIRContext which does too but I have no idea what those errors are and if they already exist in current master

Note: I have a similar PR open but for system nasm: #42266 Would be great if you can have a look or assign someone to take care of it. Change is trivial

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Aug 24, 2020
Copy link
Collaborator

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's import and see if anything breaks.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 24, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 24, 2020
@rthadur rthadur removed the ready to pull PR ready for merge process label Aug 24, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 24, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 24, 2020
@tensorflow-copybara tensorflow-copybara merged commit 65da7b8 into tensorflow:master Aug 25, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Aug 25, 2020
@Flamefire Flamefire deleted the fix_system_jsoncpp branch August 27, 2020 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:S CL Change Size: Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

Don't include json headers as include/json/json.h
8 participants