922190 - remove bundled copy of simplejson

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[:Benjamin Peterson]

Attachment: remove bundled simplejson

Python 2.7 is required to build the tree and Python 2.6+ include simplejson in the stdlib (as json), There's not much point in having a copy of simplejson sitting around in the tree when the stdlib module can just be used.

Removing python/simplejson will bring us down to one copy of simplejson in the addon sdk...

I'm attaching a patch which removes the bundled simplejson and changes all imports of simplejson to just json.

Comment 1

[Gregory Szorc [:gps]]

Comment on attachment 812117 [details] [diff] [review]
remove bundled simplejson

Review of attachment 812117 [details] [diff] [review]:
-----------------------------------------------------------------

This patch makes me happy. However, code in testing/mozbase is canonically managed at https://github.com/mozilla/mozbase. In theory, changes are supposed to land there first then merge into m-c. mozbase may also have other requirements for supporting older Python versions - I can't recall what they're up to these days.

Anyway, I'm happy with everything outside of testing/mozbase. But I'm going to hold off on r+ until jhammel chimes in with mozbase knowledge.

Comment 2

[Gregory Szorc [:gps]]

You also forgot a reference in build/virtualenv_packages.txt.

Comment 3

[Jeff Hammel]

Comment on attachment 812117 [details] [diff] [review]
remove bundled simplejson

Yes, please ticket the mozbase issue and submit a patch vs the github repo as according to https://wiki.mozilla.org/Auto-tools/Projects/Mozbase . Since all of the simplejson imports are optional, this should not block the work herein

Comment 4

[:Benjamin Peterson]

Attachment: remove bundled simplejson

Here's the patch with build/virtualenv_packages.txt fixed (I'm surprised the build didn't fail without that change) and without the mozbase changes.

Comment 5

[Gregory Szorc [:gps]]

The build didn't fail without the build/virtualenv_packages.txt change because we use .pth files and the code that creates them isn't smart enough to check the dest path exists before installing them. I hope to one day have proper packaging with proper dependencies. Our Python environment management is so lacking...

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I just want to mention that simplejson != json for Python > 2.6. Simplejson is still a separate developed module with speed improvements and more up-to-date code. The json package shipped with Python is a bit outdated.

If we don't need the speed improvements by the compiled code we could remove it. But I think we should do some perf analysis that we don't regress something for tools which have a high usage of simplejson.

Comment 7

[:Benjamin Peterson]

It's true that simplejson != json, but the stdlib json module has also seen optimization work by core CPython developers. I've never actually seen hard evidence that simplejson is significantly faster.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 812183 [details] [diff] [review]
remove bundled simplejson

Review of attachment 812183 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not too concerned about json perf issues. I don't believe there's any Python system currently generating enough JSON for this to be an issue. Although, the structured logging stuff being used could test that.

If we're really concerned about json perf, I'd be more inclined to use https://pypi.python.org/pypi/ujson, as it seems to outperform everything.

Comment 9

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2786f006654b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2786f006654b