Last Comment Bug 800557 - build shouldn't depend on particular version of simplejson, when in python 2.7 everything is in the standard library
: build shouldn't depend on particular version of simplejson, when in python 2....
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla20
Assigned To: Matěj Cepl
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-11 14:24 PDT by Matěj Cepl
Modified: 2012-11-27 16:24 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
suggested patch (1.46 KB, patch)
2012-10-11 14:24 PDT, Matěj Cepl
nfroyd: review+
Details | Diff | Splinter Review
mozconfig used (234 bytes, text/plain)
2012-10-11 14:25 PDT, Matěj Cepl
no flags Details
requested file (15.65 KB, text/plain)
2012-11-15 06:28 PST, Matěj Cepl
no flags Details
New version of the patch (3.56 KB, patch)
2012-11-16 02:58 PST, Matěj Cepl
nfroyd: review+
Details | Diff | Splinter Review

Description Matěj Cepl 2012-10-11 14:24:41 PDT
Created attachment 670542 [details] [diff] [review]
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 2012-10-11 14:25:36 PDT
Created attachment 670544 [details]
mozconfig used
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-10-11 15:07:23 PDT
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] 2012-10-11 15:30:42 PDT
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] 2012-10-11 15:32:54 PDT
This could be related to bug 789901
Comment 5 Jeff Hammel 2012-10-11 15:35:26 PDT
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 2012-10-11 15:37:51 PDT
(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 2012-10-11 15:39:23 PDT
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] 2012-10-11 15:44:39 PDT
(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 2012-10-11 15:50:49 PDT
(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] 2012-10-11 15:51:36 PDT
(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] 2012-10-11 15:52:32 PDT
(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] 2012-10-11 15:59:50 PDT
(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] 2012-10-11 16:00:55 PDT
Bug 800614 filed to discuss updating the minimum Python version to build the tree.
Comment 14 Matěj Cepl 2012-11-15 05:02:14 PST
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] 2012-11-15 05:16:22 PST
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] 2012-11-15 05:19:35 PST
Actually, what about comment 5?  Won't this patch break building with earlier python versions until we officially require 2.7?
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2012-11-15 05:42:33 PST
(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 2012-11-15 06:08:48 PST
(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] 2012-11-15 06:26:49 PST
(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 2012-11-15 06:28:26 PST
Created attachment 681970 [details]
requested file

This is the file which was generated for me. What do you think?
Comment 21 Matěj Cepl 2012-11-15 06:28:26 PST
Created attachment 681970 [details]
requested file

This is the file which was generated for me. What do you think?
Comment 22 Nathan Froyd [:froydnj] 2012-11-15 06:34:05 PST
(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 Ted Mielczarek [:ted.mielczarek] 2012-11-15 07:40:31 PST
"import json" by itself is fine, that works in Python 2.6.
Comment 24 Matěj Cepl 2012-11-15 08:54:13 PST
What's the next step here? Should I ask for checkin? Whom?
Comment 25 Ted Mielczarek [:ted.mielczarek] 2012-11-15 09:16:43 PST
If you add the checkin-needed keyword the magical checkin fairies will land your patch for you. :)
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-11-15 17:41:22 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2469a8b41a
Comment 28 Gary Kwong [:gkw] [:nth10sd] 2012-11-15 18:32:18 PST
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] 2012-11-15 18:38:09 PST
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 2012-11-16 02:58:45 PST
Created attachment 682400 [details] [diff] [review]
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] 2012-11-16 06:29:08 PST
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 2012-11-24 06:39:46 PST
Ah, err ... I missed that :ted hasn't reviewed the patch yet.
Comment 33 Ted Mielczarek [:ted.mielczarek] 2012-11-27 11:03:47 PST
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] 2012-11-27 11:56:05 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/16dc855a0216
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-11-27 16:24:15 PST
https://hg.mozilla.org/mozilla-central/rev/16dc855a0216

Note You need to log in before you can comment on or make changes to this bug.