Skip to content
This repository has been archived by the owner on Feb 13, 2024. It is now read-only.

[Do Not Merge][LibOverhaul 03] Update JsonCpp #3338

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MartinBriza
Copy link
Contributor

@MartinBriza MartinBriza commented Sep 20, 2019

📒 Description

To state the obvious, JsonCpp is a third party library we're using to parse Json in the library.
We were using quite an outdated version so I updated it to the latest release (1.9.1).
This also means we had to change the way we're using the library (the classes we used to use are now deprecated) so I added a helper class so we don't have to put in a lot of boilerplate code.

🕶️ Types of changes

  • New feature (non-breaking change which adds functionality)

🤯 List of changes

  • JsonCpp dependency has been updated

Closes #4463

🔎 Review hints

  • Same as with most library overhaul PRs, warnings should disappear and the app should continue to compile and work the same on all platforms.

@NghiaTranUIT NghiaTranUIT self-requested a review September 23, 2019 08:43
@NghiaTranUIT
Copy link
Contributor

Basically, this PR works well on all platforms. I double-checked all primary features and it's as we expected.

However, I encountered the crash when trying logging out. The log is pointing to JSON Parser. Therefore, I suppose we should take a look at it.

How to reproduce

  1. Check out this PR, and login
  2. Start any TE and wait 10s
  3. Stop and quickly log-out
  4. The crash happens 💥

Screenshots

Screen Shot 2019-09-26 at 14 13 43

2019-09-26 14 21 57

Copy link
Contributor

@NghiaTranUIT NghiaTranUIT left a comment

Choose a reason for hiding this comment

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

Please investigate ☝️ issues

@MartinBriza
Copy link
Contributor Author

I spent a good while testing this today and it crashes on random locations. I'm starting to suspect I discovered another race condition in the library....

@MartinBriza MartinBriza changed the title [LibOverhaul 03] Update JsonCpp [Do Not Merge][LibOverhaul 03] Update JsonCpp Nov 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Our embedded jsoncpp is old
2 participants