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

RESOLVED FIXED in mozilla20

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Matěj Cepl, Assigned: Matěj Cepl)

Tracking

Trunk
mozilla20
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 670544 [details]
mozconfig used
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

5 years ago
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.
This could be related to bug 789901

Comment 5

5 years ago
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

5 years ago
(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

5 years ago
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

5 years ago
(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

5 years ago
(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
(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.
(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.
(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.
Bug 800614 filed to discuss updating the minimum Python version to build the tree.
(Assignee)

Comment 14

5 years ago
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?
(Assignee)

Updated

5 years ago
Attachment #670542 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Attachment #670542 - Flags: review?(gps) → review?(nfroyd)
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.
Attachment #670542 - Flags: review?(nfroyd) → review+
Actually, what about comment 5?  Won't this patch break building with earlier python versions until we officially require 2.7?
(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.
(Assignee)

Comment 18

5 years ago
(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.
(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.
(Assignee)

Comment 20

5 years ago
Created attachment 681970 [details]
requested file

This is the file which was generated for me. What do you think?
Flags: needinfo?(nfroyd)
(Assignee)

Comment 21

5 years ago
Created attachment 681970 [details]
requested file

This is the file which was generated for me. What do you think?
(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!
Flags: needinfo?(nfroyd)
"import json" by itself is fine, that works in Python 2.6.
Assignee: nobody → mcepl
(Assignee)

Comment 24

5 years ago
What's the next step here? Should I ask for checkin? Whom?
If you add the checkin-needed keyword the magical checkin fairies will land your patch for you. :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2469a8b41a
Keywords: checkin-needed
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
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'
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).
(Assignee)

Comment 30

5 years ago
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.
Attachment #682400 - Flags: review?(ted)
Attachment #682400 - Flags: review?(nfroyd)
(Assignee)

Updated

5 years ago
Attachment #670542 - Attachment is obsolete: true
Comment on attachment 682400 [details] [diff] [review]
New version of the patch

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

Works for me.
Attachment #682400 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 32

5 years ago
Ah, err ... I missed that :ted hasn't reviewed the patch yet.
Keywords: checkin-needed
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.
Attachment #682400 - Flags: review?(ted)
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/16dc855a0216
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/16dc855a0216
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.