647404 - automation.py: fix extractZip() and installExtension() issues from bug 573263, affecting bug 647394

Status: VERIFIED FIXED

(Testing :: General, defect)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) s|os.sep|'/'|, Add shutil.rmtree() call, Fix some code nits [Checked in: Comment 14]

shutil.rmtree(): tell me if you want it in a try+finally.

'/': "unexpected" issue, but unless it's my Windows 2000 etc only...

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Rewrite installExtension() to copy xpi file without extracting it, Remove not needed anymore extractZip()

Per bug 613109 comment 3 and comment 4...

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 523761 [details] [diff] [review]
(Bv1) Rewrite installExtension() to copy xpi file without extracting it, Remove not needed anymore extractZip()

This is cleaner, faster and works better:
using --install-extension= with a relative path+filename instead of a filename only does work now ;->
(See bug 647394 for such a case.)

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 523754 [details] [diff] [review]
(Av1) s|os.sep|'/'|, Add shutil.rmtree() call, Fix some code nits
[Checked in: Comment 14]

NB: I'm not sure why this code does extract2temp+copy2dest rather than extract2dest, but this first patch is just to make it work as is.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Explicitly support an optional path in --install-extension

(In reply to comment #3)
> using --install-extension= with a relative path+filename instead of a filename
> only does work now ;->

Not too sure how it works as is, but let's do it right.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 523754 [details] [diff] [review]
(Av1) s|os.sep|'/'|, Add shutil.rmtree() call, Fix some code nits
[Checked in: Comment 14]

thanks for the cleanup!

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 523761 [details] [diff] [review]
(Bv1) Rewrite installExtension() to copy xpi file without extracting it, Remove not needed anymore extractZip()

>+      # Copy extension tree into its own directory.
>+      shutil.copytree(extensionSource, os.path.join(extnsdir, extensionID))

here we are assuming that extensionSource is a directory.  Can we verify that it exists before copytree?

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 523834 [details] [diff] [review]
(Cv1) Explicitly support an optional path in --install-extension

>       abspath = self.getFullPath(f)
>-      extensionID = f[:f.rfind(".")]
>+      extensionID = abspath[abspath.rfind(os.sep) + 1 : abspath.rfind(".")]
>       self.automation.installExtension(abspath, profileDir, extensionID)

does os.sep work correctly in msys (our shell for windows)?  


>       abspath = self.getFullPath(f)
>-      extensionID = f[:f.rfind(".")]
>+      extensionID = abspath[abspath.rfind(os.sep) + 1 : abspath.rfind(".")]
>       self.automation.installExtension(abspath, options.profilePath, extensionID)

same as above

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) Rewrite installExtension() to copy xpi file without extracting it, Remove now unused extractZip() [Checked in: Comment 15]

Bv1, with comment 7 suggestion(s).

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> does os.sep work correctly in msys (our shell for windows)?  

Yes, that's how I'm testing it: Windows 2000 + msys.

Comment 11

[(no longer active)]

Please renom for landing once all of the patches are reviewed.

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2) Explicitly support an optional path in --install-extension [Checked in: Comment 16]

Cv1, unbloated.


(In reply to comment #5)
> Not too sure how it works as is, but let's do it right.

It works with patch B because extensionID isn't used anymore :->

Comment 13

[(no longer active)]

(In reply to comment #11)
> Please renom for landing once all of the patches are reviewed.

Still not changing my position here!  ;-)

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 523754 [details] [diff] [review]
(Av1) s|os.sep|'/'|, Add shutil.rmtree() call, Fix some code nits
[Checked in: Comment 14]

http://hg.mozilla.org/mozilla-central/rev/66656b3d818f

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 524044 [details] [diff] [review]
(Bv2) Rewrite installExtension() to copy xpi file without extracting it, Remove now unused extractZip()
[Checked in: Comment 15]

http://hg.mozilla.org/mozilla-central/rev/a6afe12a2896

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 524670 [details] [diff] [review]
(Cv2) Explicitly support an optional path in --install-extension
[Checked in: Comment 16]

http://hg.mozilla.org/mozilla-central/rev/3afa19c50a85

Comment 17

[(no longer active)]

Any reason why these patches are needed on mozilla-2.1?  IINM the only product being released from that component is Fennec.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)

> Any reason why these patches are needed on mozilla-2.1?

No other reason (that I know of) than m-2.1 is in-between m-c and m-2.0, where SeaMonkey 2.1 needs this.

> IINM the only product being released from that component is Fennec.

I was told so once.
Ftr, I'm still trying to recover from what happened with m-2.0, let alone trying to figure out m-2.1 story :-|

Comment 19

[(no longer active)]

So, here's what's happening with the 2.1 repo.  Mobile guys will release dot-released to Fennec 4 off of that repo.  As far as I know, no other project is going to use it.  Hence, I don't think we need to land this on 2.1.

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> Hence, I don't think we need to land this on 2.1.

Let me say in other words what I'm looking for:
I understand about m-c and m-2.0 (and m-1.9.*).
I'm still very unclear about all that is happening "in-between" (or aside of!?) m-c and m-2.0, as in m-2.1, m-aurora, etc, or the various branch code names, as in "birsh", "cedar", etc, or where FF5 actually lives now.
My goal is to have my patches checked in to m-2.0 (so SeaMonkey 2.1 can have it), and to any repository that future SeaMonkey versions might be released off.
Hence, this should be checked in to all "in-between" repositories that apply (if any).

NB: I'm talking about SeaMonkey, but this bug may be useful to other applications, like testing Firefox+Firebug for example (if that's still wanted).

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #20)
> My goal is to have my patches checked in to m-2.0 (so SeaMonkey 2.1 can have
> it), and to any repository that future SeaMonkey versions might be released
> off.

Hum, let me try in yet other words:
I was even more surprised by the jump to Gecko versions 5 and 6...
But maybe that's just the point, iiuc.
I checked in to m-c which is now Gecko 6.
I need this in m-2.0 which is Gecko 4.
Then maybe there's just 1 "in-between" repo to care about, the one for Gecko 5 ... which currently is what, m-aurora!?

Comment 22

[LegNeato]

We don't need this for FF5 and don't believe there are plans to release SM out of FF5....tracking -.

(note it is still an open question if it will be taken on releases/mozilla-2.0)

Comment 23

[(no longer active)]

You may find this document helpful: <http://mozilla.github.com/process-releases/draft/development_specifics/>

Comment 24

[LegNeato]

(we will also be adding the bug process and tree rules there shortly as well)

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> (note it is still an open question if it will be taken on releases/mozilla-2.0)

Any progress?
This is, for example, blocking bug 647414 which blocks bug 647394...

Comment 26

[Serge Gautherie (:sgautherie)]

V.Fixed, per bug 647414 comment 14.

Comment 27

[Justin Wood (:Callek)]

(In reply to comment #22)
> We don't need this for FF5 and don't believe there are plans to release SM
> out of FF5....tracking -.

SeaMonkey is (now that we have discussed) planning to jump directly onto the train, and release off Firefox 5...

> (note it is still an open question if it will be taken on
> releases/mozilla-2.0)

As a SeaMonkey Driver, I do not feel taking this on m-2.0 (or even aurora for Firefox 5) is worth the efforts/risk. This is needed for test runs, and nothing else.

Comment 28

[Serge Gautherie (:sgautherie)]

(In reply to comment #23)
> You may find this document helpful:
> <http://mozilla.github.com/process-releases/draft/development_specifics/>

Yeah, I had already tried to read this draft once or twice.

Now that things have settled a little, iiuc, after having had an overly long FF4 cycle (including tree "freeze"), we're now just having a release-and-forget mozilla2 branch, a Fennec only mozilla2.1 branch and a very quick frozen-open-frozen mozilla5 branch.
This explains why I had a hard time figuring out where/when to land non Firefox patches: the answer seems to mostly be to jump from 2.0pre to 6.0pre :-<
So be it.