Swap from FFJSON to easyjson #1322
Conversation
|
Should fix #1283 |
|
If we make this change we should also look at doing this in buildah and containers/storage? |
|
Hm. It seems like we might need to vendor the easyjson repo (the generated files seem to be referring back to it). @rhatdan It might be worth looking into, but it's not really a big real for c/storage and buildah because they've used FFJSON exclusively for a long time, and slight encoding/decoding differences vs encoding/json don't matter as much there. Here, we do need near-exact compatibility because we're transitioning an existing database created with encoding/json. |
|
I'm not really sure why travis is showing as pending here - it's green when you actually click on it... |
|
☔ The latest upstream changes (presumably 63dd200) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably 4c00dc6) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Sadly needs a rebase. |
Signed-off-by: Matthew Heon <matthew.heon@gmail.com>
FFJSON has serialization differences versus stock Go - namely, it does not respect the MarshalText() and UnmarshalText() methods, particularly on []byte, which causes incompatability with pre-FFJSON containers which contained DNS servers. EasyJSON does not have these issues, and might even be slightly faster. Signed-off-by: Matthew Heon <matthew.heon@gmail.com>
To ensure we can build without easyjson installed, vendor the easyjson repository as the generated files use the easyjson library. Signed-off-by: Matthew Heon <matthew.heon@gmail.com>
Signed-off-by: Matthew Heon <matthew.heon@gmail.com>
|
Rebased |
|
LGTM |
|
bot, retest this please |
|
📌 Commit 367541e has been approved by |
FFJSON has serialization differences versus stock Go - namely, it does not respect the MarshalText() and UnmarshalText() methods, particularly on []byte, which causes incompatability with pre-FFJSON containers which contained DNS servers. EasyJSON does not have these issues, and might even be slightly faster. Signed-off-by: Matthew Heon <matthew.heon@gmail.com> Closes: #1322 Approved by: mheon
To ensure we can build without easyjson installed, vendor the easyjson repository as the generated files use the easyjson library. Signed-off-by: Matthew Heon <matthew.heon@gmail.com> Closes: #1322 Approved by: mheon
Signed-off-by: Matthew Heon <matthew.heon@gmail.com> Closes: #1322 Approved by: mheon
|
☀️ Test successful - status-papr |
github-actions
bot
added
the
locked - please file new issue/PR
FFJSON has serialization issues related to
MarshalText/UnmarshalTextthat most notably affect serializingnet.IPand cause incompatibility with containers created with DNS servers prior to FFJSON being re-added.I investigated adding support for these to FFJSON, but didn't make much progress. Swapping to an alternative JSON generator seems like a better option.
Caveat: easyjson is newer and less mature, and potentially more buggy in other areas.