1674162 - Use of deprecated function `plistlib.readPlist()` in `mozinstall.py`

Status: RESOLVED FIXED

(Testing :: Mozbase, defect, P3)

Description

[Ricky Stewart]

We import this function which was deprecated in Python 3.4 and deleted in Python 3.9. We should be using plist.load() instead.

Comment 1

[Ricky Stewart]

This patch seems fine, but I don't know how to test it.

diff --git a/testing/mozbase/mozinstall/mozinstall/mozinstall.py b/testing/mozbase/mozinstall/mozinstall/mozinstall.py
index 4b6231888c7f..7822da5c123b 100644
--- a/testing/mozbase/mozinstall/mozinstall/mozinstall.py
+++ b/testing/mozbase/mozinstall/mozinstall/mozinstall.py
@@ -28,9 +28,6 @@ try:
 except ImportError:
     has_pefile = False
 
-if mozinfo.isMac:
-    from plistlib import readPlist
-
 
 TIMEOUT_UNINSTALL = 60
 
@@ -69,13 +66,15 @@ def get_binary(path, app_name):
 
     # On OS X we can get the real binary from the app bundle
     if mozinfo.isMac:
+        import plistlib
         plist = "%s/Contents/Info.plist" % path
         if not os.path.isfile(plist):
             raise InvalidBinary("%s/Contents/Info.plist not found" % path)
 
-        binary = os.path.join(
-            path, "Contents/MacOS/", readPlist(plist)["CFBundleExecutable"]
-        )
+        with open(plist, "rb") as fp:
+            binary = os.path.join(
+                path, "Contents/MacOS/", plistlib.load(fp)["CFBundleExecutable"]
+            )
 
     else:
         app_name = app_name.lower()

Comment 2

[Bob Clary [:bc] (inactive)]

I think you can remove the skip-if = os == "mac" lines in mozinstall/tests/manifest.ini and try mach python-test testing/mozbase/mozinstall/.

I'm not a big fan of imports inside functions. I think it would be better if we conditionally imported plistlib and defined a new function readPlist at global scope. What do you think?

Comment 3

[Ricky Stewart]

Attachment: Bug 1674162 - Remove reference to deleted function `plistlib.readPlist()`

Comment 5

[Mike Hommey [:glandium]]

Sorry, same problem, different file.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f3d1f4e24e2
Remove reference to deleted function `plistlib.readPlist()` r=bc

Comment 7

[Narcis Beleuzu [:NarcisB]]

Backed out for failures on mozinstall.py

Backout link: https://hg.mozilla.org/integration/autoland/rev/5ead1f96fb8c0301d0002fca772aa6e29c337df0
Log link: https://treeherder.mozilla.org/logviewer?job_id=320311560&repo=autoland&lineNumber=1331

Comment 9

[Ricky Stewart]

lol

Comment 11

[Gabriele Svelto [:gsvelto]]

I just ran into this while trying to bootstrap under macOS, this breaks bootstrap when downloading nodejs

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bad32157ee4d
Remove reference to deleted function `plistlib.readPlist()` r=bc

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/bad32157ee4d

Comment 14

[Mitchell Hentges [:mhentges] 🦀]

Comment on attachment 9184645 [details]
Bug 1674162 - Remove reference to deleted function plistlib.readPlist()

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: ESR78 doesn't build for downstream developers with Python 3.9.
• User impact if declined: They will have to downgrade their python (to 3.8).
• Fix Landed on Version: 84
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Build patch.
• String or UUID changes made by this patch:

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9184645 [details]
Bug 1674162 - Remove reference to deleted function plistlib.readPlist()

Approved for 78.8esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/fe44415d9ad1