Merge lp:~vila/bzr/712474-module-available into lp:bzr

[Vincent Ladeuil]

ModuleAvailableFeature don't try to imported already imported modules.

__import__ doesn't check that a module has already been imported, don't use it.

See the bug report for a real life use case (bzr-svn plugin).

[Martin Packman (gz)]

I think checking sys.modules should do instead, something like:

    sentinel = object()
    module = sys.modules.get(self.module_name)
    if module is None:
        return False
    if module is sentinel:
        try:
            module = __import__(self.module_name, {}, {}, [''])
        except ImportError: # import can raise other things, but that's a bug
            return False
    return True

That should save me worrying needlessly about exec on an interpolated string.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/3/2011 9:44 AM, Martin [gz] wrote:
> I think checking sys.modules should do instead, something like:
>
> sentinel = object()
> module = sys.modules.get(self.module_name)
> if module is None:
> return False
> if module is sentinel:
> try:
> module = __import__(self.module_name, {}, {}, [''])
> except ImportError: # import can raise other things, but that's a bug
> return False
> return True
>
> That should save me worrying needlessly about exec on an interpolated string.

I think you wanted

module = sys.modules.get(self.module_name, sentinel)

And regardless, this should probably be pulled out into a separate
helper function. I think the bzrlib.plugin code would like something
like this. Though we can see how related it is.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1Kz4sACgkQJdeBCYSNAAPLjgCfaPi842OVTWonJ6f9yKDI6gCg
udAAoKV/XVWEaH+Srs7NS/bsm0VqwA+u
=ZTnz
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

We'd really like to get away from "exec 'import %s'" if you can have it done a different way. I'm not particularly worried about code injection because importing a plugin can do whatever the heck it wants anyway.

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

ping?

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/16/2011 4:21 PM, Vincent Ladeuil wrote:
> You have been requested to review the proposed merge of
> lp:~vila/bzr/712474-module-available into lp:bzr.
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr/712474-module-available/+merge/48474
>
> __import__ doesn't check that a module has already been imported,
> don't use it.
>
> See the bug report for a real life use case (bzr-svn plugin).
>
>

Your cover letter is a bit incorrect. You're still using "__import__"
you're just guarding it with a check that the module isn't already imported.

 merge: approve

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5KgpwACgkQJdeBCYSNAAN9OgCeIiAvxAQ57XRprJEOikfhFbff
4akAniys+2FQugggnC0uV2pLm4ISxJYQ
=Sf+E
-----END PGP SIGNATURE-----

review: Approve

[Vincent Ladeuil (vila)]

> Your cover letter is a bit incorrect. You're still using "__import__"

But never twice for the same module.

> you're just guarding it with a check that the module isn't already imported.

Indeed. Anyway, rephrased to avoid confusion.

[Vincent Ladeuil (vila)]

Ha, *cover* letter, yeah sorry, I didn't update it.

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/tests/features.py'
--- bzrlib/tests/features.py	2011-08-11 10:09:27 +0000
+++ bzrlib/tests/features.py	2011-08-16 15:21:51 +0000
@@ -166,11 +166,17 @@
         self.module_name = module_name
 
     def _probe(self):
-        try:
-            self._module = __import__(self.module_name, {}, {}, [''])
+        sentinel = object()
+        module = sys.modules.get(self.module_name, sentinel)
+        if module is sentinel:
+            try:
+                self._module = __import__(self.module_name, {}, {}, [''])
+                return True
+            except ImportError:
+                return False
+        else:
+            self._module = module
             return True
-        except ImportError:
-            return False
 
     @property
     def module(self):

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-08-16 09:33:16 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-08-16 15:21:51 +0000
@@ -196,6 +196,10 @@
 * ``LockDir`` can now be run when the local hostname is ``localhost``.
   (Jelmer Vernooij, #825994)
 
+* ``ModuleAvailableFeature`` won't try to import already imported modules,
+  allowing it to be used for modules with side-effects.
+  (Vincent Ladeuil, #712474)
+
 * `TestCaseWithMemoryTransport` is faster now: `_check_safety_net` now
   just compares the bytes in the dirstate file to its pristine state,
   rather than opening the WorkingTree and calling ``last_revision()``.