800557 - build shouldn't depend on particular version of simplejson, when in python 2.7 everything is in the standard library

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Matěj Cepl]

Attachment: suggested patch

When building mozilla-central/master (from https://github.com/mozilla/mozilla-central) on a system with simplejson 1.9.2 I got a build error:

Traceback (most recent call last):
  File "/home/matej/archiv/knihovna/repos/gaia/src/toolkit/components/telemetry/gen-histogram-enum.py", line 32, in <module>
    main(sys.argv[1:])
  File "/home/matej/archiv/knihovna/repos/gaia/src/toolkit/components/telemetry/gen-histogram-enum.py", line 22, in main
    for histogram in histogram_tools.from_file(filename):
  File "/home/matej/archiv/knihovna/repos/gaia/src/toolkit/components/telemetry/histogram_tools.py", line 201, in from_file
    histograms = json.load(f, object_pairs_hook=json.OrderedDict)
AttributeError: 'module' object has no attribute 'OrderedDict'
make[7]: *** [TelemetryHistogramEnums.h] Error 1
make[7]: *** Deleting file `TelemetryHistogramEnums.h'
make[7]: *** Waiting for unfinished jobs....
make[7]: Leaving directory `/home/matej/archiv/knihovna/repos/gaia/build/toolkit/components/telemetry'
make[6]: *** [telemetry_export] Error 2
make[6]: Leaving directory `/home/matej/archiv/knihovna/repos/gaia/build/toolkit/components'
make[5]: *** [components_export] Error 2
make[5]: Leaving directory `/home/matej/archiv/knihovna/repos/gaia/build/toolkit'
make[4]: *** [export_tier_platform] Error 2
make[4]: Leaving directory `/home/matej/archiv/knihovna/repos/gaia/build'
make[3]: *** [tier_platform] Error 2
make[3]: Leaving directory `/home/matej/archiv/knihovna/repos/gaia/build'
make[2]: *** [default] Error 2
make[2]: Leaving directory `/home/matej/archiv/knihovna/repos/gaia/build'
make[1]: *** [realbuild] Error 2
make[1]: Leaving directory `/home/matej/archiv/knihovna/repos/gaia/src'
make: *** [build] Error 2

However, simplejson.OrderedDict is not needed at all, actually whole simplejson is not needed, because all what's required (OrderedDict and json) are part of the standard library in python 2.7.

Suggested patch tries to use standard library only, and only when it fails (with python <= 2.6) it uses simplejson library.

Comment 1

[Matěj Cepl]

Attachment: mozconfig used

Comment 2

[(not currently active) Ted Mielczarek]

We should be installing simplejson 2.1.1 into the in-tree virtualenv:
http://mxr.mozilla.org/mozilla-central/source/python/simplejson-2.1.1/

Comment 3

[Gregory Szorc [:gps]]

If all the builders are running Python 2.7 now, perhaps we should just bump the minimum required Python version to 2.7.2 (because MozillaBuild) and remove this simplejson nonsense from the tree.

Comment 4

[Mike Hommey [:glandium]]

This could be related to bug 789901

Comment 5

[Jeff Hammel]

So part of the problem is that histogram_tools.py imports directly from simplejson:

https://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/histogram_tools.py#8

The standard pattern, which we should use universally until we no longer support e.g. python 2.5, is:

try:
    import json
except ImportError:
    import simplejson as json

Comment 6

[Jeff Hammel]

(In reply to Gregory Szorc [:gps] from comment #3)
> If all the builders are running Python 2.7 now, perhaps we should just bump
> the minimum required Python version to 2.7.2 (because MozillaBuild) and
> remove this simplejson nonsense from the tree.

I'm also fine with this, assuming nothing breaks.  Since json was introduced in 2.6 we don't even need to go that far.  If we are deprecating 2.5, though, shouldn't we:

- talk about it somewhere?
- have it written somewhere in bold where people (like me) can find it?

Comment 7

[Jeff Hammel]

Comment on attachment 670542 [details] [diff] [review]
suggested patch

This won't work on 2.5;  you'll also need the

try:
    import json
except:
    import simplejson as json

Comment 8

[Gregory Szorc [:gps]]

(In reply to Jeff Hammel [:jhammel] from comment #6)
> I'm also fine with this, assuming nothing breaks.  Since json was introduced
> in 2.6 we don't even need to go that far.  If we are deprecating 2.5,
> though, shouldn't we:
> 
> - talk about it somewhere?

https://groups.google.com/d/topic/mozilla.dev.platform/djN02O03APc/discussion

There were no strong objections. I may just throw up a patch to bump it to 2.6. Let me file a bug...

Comment 9

[Jeff Hammel]

(In reply to Gregory Szorc [:gps] from comment #8)
> (In reply to Jeff Hammel [:jhammel] from comment #6)
> > I'm also fine with this, assuming nothing breaks.  Since json was introduced
> > in 2.6 we don't even need to go that far.  If we are deprecating 2.5,
> > though, shouldn't we:
> > 
> > - talk about it somewhere?
> 
> https://groups.google.com/d/topic/mozilla.dev.platform/djN02O03APc/discussion
> 
> There were no strong objections. I may just throw up a patch to bump it to
> 2.6. Let me file a bug...

WFM, be sure you put this information somewhere (damned if i know where, i'd put it in https://developer.mozilla.org/en-US/docs/Python and probably other places that MDN search is failing for me right now) and round out the thread

Comment 10

[Mike Hommey [:glandium]]

(In reply to Jeff Hammel [:jhammel] from comment #7)
> Comment on attachment 670542 [details] [diff] [review]
> suggested patch
> 
> This won't work on 2.5;  you'll also need the
> 
> try:
>     import json
> except:
>     import simplejson as json

I doubt json provides OrderedDict.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #10)
> I doubt json provides OrderedDict.

Forget this comment, I didn't see this was in reply to the patch.

Comment 12

[Mike Hommey [:glandium]]

(In reply to Jeff Hammel [:jhammel] from comment #6)
> Since json was introduced in 2.6 we don't even need to go that far.

telemetry scripts as well as dom/imptests/parseFailures.py and bug 780561 use OrderedDict.
Telemetry scripts and bug 780561 use the copy in simplejson, and dom/imptests/parseFailures.py uses collections.OrderedDict, which means that in practice, it's already a python 2.7 only script.
Anyways, OrderedDict is only available in 2.7, as well as many other good things. If we're going to bump, that should be 2.7, really.

Comment 13

[Gregory Szorc [:gps]]

Bug 800614 filed to discuss updating the minimum Python version to build the tree.

Comment 14

[Matěj Cepl]

So, if I understand bug 800614 correctly we depend on python 2.7, but nevertheless we still require simplejson (at least commit 7d22135 just FTBFS on me for not having simplejson). Why?

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 670542 [details] [diff] [review]
suggested patch

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

r=me if you double-check that $OBJDIR/toolkit/components/telemetry/TelemetryHistogramEnums.h is the same before and after this change.

Comment 16

[Nathan Froyd [:froydnj]]

Actually, what about comment 5?  Won't this patch break building with earlier python versions until we officially require 2.7?

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Nathan Froyd (:froydnj) from comment #16)
> Actually, what about comment 5?  Won't this patch break building with
> earlier python versions until we officially require 2.7?

json is in the standard library since 2.6; collections.OrderedDict since 2.7, AIUI.

Comment 18

[Matěj Cepl]

(In reply to Nathan Froyd (:froydnj) from comment #16)
> Actually, what about comment 5?  Won't this patch break building with
> earlier python versions until we officially require 2.7?

See comment 12 ... we actually already depends on 2.7.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Matej Cepl from comment #18)
> (In reply to Nathan Froyd (:froydnj) from comment #16)
> > Actually, what about comment 5?  Won't this patch break building with
> > earlier python versions until we officially require 2.7?
> 
> See comment 12 ... we actually already depends on 2.7.

Only for dom/imptests/parseFailures.py, and i don't think it runs during a build.

Comment 20

[Matěj Cepl]

Attachment: requested file

This is the file which was generated for me. What do you think?

Comment 21

[Matěj Cepl]

Attachment: requested file

This is the file which was generated for me. What do you think?

Comment 22

[Nathan Froyd [:froydnj]]

(In reply to Matej Cepl from comment #21)
> This is the file which was generated for me. What do you think?

Looks good to me.  Please do modify the patch appropriately as to whether:

import json

is sufficient or such an import needs to be checked--I'll let people more qualified than I come to a consensus on that matter.  (No re-review required for that change.)  Thanks!

Comment 23

[(not currently active) Ted Mielczarek]

"import json" by itself is fine, that works in Python 2.6.

Comment 24

[Matěj Cepl]

What's the next step here? Should I ask for checkin? Whom?

Comment 25

[(not currently active) Ted Mielczarek]

If you add the checkin-needed keyword the magical checkin fairies will land your patch for you. :)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2469a8b41a

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0cd4bcb3217

https://tbpl.mozilla.org/php/getParsedLog.php?id=17086276&tree=Mozilla-Inbound

Comment 28

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Traceback from the log:

Traceback (most recent call last):
  File "e:/builds/moz2_slave/m-in-w32/build/toolkit/components/telemetry/gen-histogram-enum.py", line 32, in <module>
    main(sys.argv[1:])
  File "e:/builds/moz2_slave/m-in-w32/build/toolkit/components/telemetry/gen-histogram-enum.py", line 22, in main
    for histogram in histogram_tools.from_file(filename):
  File "e:\builds\moz2_slave\m-in-w32\build\toolkit\components\telemetry\histogram_tools.py", line 205, in from_file
    histograms = json.load(f, object_pairs_hook=OrderedDict)
  File "c:\mozilla-build\python\Lib\json\__init__.py", line 267, in load
    parse_constant=parse_constant, **kw)
  File "c:\mozilla-build\python\Lib\json\__init__.py", line 318, in loads
    return cls(encoding=encoding, **kw).decode(s)
TypeError: __init__() got an unexpected keyword argument 'object_pairs_hook'

Comment 29

[Gregory Szorc [:gps]]

Well, I guess this is part of the build after all! FWIW, the Windows builders are still on Python 2.6, which is why this is failing (object_pairs_hook appears to have been added in Python 2.7).

Comment 30

[Matěj Cepl]

Attachment: New version of the patch

Hopefully this should make this directory working even with Python 2.6 (and of course, then simplejson is required).

Also removed two imports of with_statement from __future__ (it is mandatory part of 2.6).

Generally, when looking for other simplejsons in the Mozilla sources I found this:

wycliff:src (master) $ grep -r simplejson --include=\*.py .|grep -v simplejson-2.1.1
./toolkit/components/telemetry/gen-histogram-bucket-ranges.py:import simplejson as json
./toolkit/components/telemetry/histogram_tools.py:import simplejson as json
./js/src/devtools/gc/gc-test.py:            import simplejson as json
./js/src/tests/compare_bench.py:    import simplejson as json
./build/unix/build-clang/tooltool.py:    import simplejson as json # I hear simplejson is faster
./build/unix/build-clang/build-clang.py:import simplejson
./build/unix/build-clang/build-clang.py:    data = simplejson.load(file(manifest))
./build/unix/build-clang/build-clang.py:    simplejson.dump(data, out, indent=0, item_sort_key=key_sort)
./testing/talos/talos_from_code.py:    import simplejson as json
./testing/mozbase/moztest/setup.py:    deps.append('simplejson')
./testing/mozbase/mozprofile/mozprofile/profile.py:    import simplejson
./testing/mozbase/mozprofile/mozprofile/profile.py:    import json as simplejson
./testing/mozbase/mozprofile/mozprofile/profile.py:            _prefs = [(simplejson.dumps(k), simplejson.dumps(v) )
./testing/mozbase/mozprofile/mozprofile/prefs.py:    import simplejson as json
./testing/mozbase/mozprofile/setup.py:    deps.append('simplejson')
./testing/mozbase/mozinfo/mozinfo/mozinfo.py:                from simplejson import loads
./testing/mozbase/mozinfo/setup.py:    deps = ['simplejson']
./testing/mozbase/mozhttpd/tests/api.py:    import simplejson as json
./testing/mozbase/mozhttpd/mozhttpd/handlers.py:    import simplejson as json
./testing/peptest/peptest/runpeptests.py:    import simplejson as json
wycliff:src (master) $ 

I guess these are truly not used during the build and we could deal with them later (when 2.6 finally truly fades away).

Although some of them obviously prefer simplejson over json (some in quite bizarre way ... try_import_json in js/src/devtools/gc/gc-test.py), I believe (http://bugs.python.org/issue4136) it is truly not necessary for 2.7, because json standard library has been rebased against the simplejson. However, as long as 2.6 is on the scene, I believe we should leave it as it is.

Comment 31

[Nathan Froyd [:froydnj]]

Comment on attachment 682400 [details] [diff] [review]
New version of the patch

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

Works for me.

Comment 32

[Matěj Cepl]

Ah, err ... I missed that :ted hasn't reviewed the patch yet.

Comment 33

[(not currently active) Ted Mielczarek]

Comment on attachment 682400 [details] [diff] [review]
New version of the patch

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

Nathan's review is enough here. Sorry I didn't look at this sooner.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16dc855a0216

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16dc855a0216