660427 - Update (re)packaging code to deal with extracting the installer vs just the zip.

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Justin Wood (:Callek)]

Attachment: v0.5

The mozilla-side omnijar code uses the .exe to repackage installer, and that code also changed the unpack steps for the complete-mar generation during repackaging.

I have a patch, want feedback.

So far I have only tested that it *seems* to do what I want, and "compiles" for a repack locally, did not yet run the updates/repack tests on it.

Assigning feedback requests to a few people, since this *is* complex code. Patch is against m-2.0, but the rapid-release-repos have not changed much in this codepath afaict.

Comment 1

[Philip Chee]

Why the change from double quotes to single quotes?

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why are you changing the nsinstall manifest?

Comment 3

[Justin Wood (:Callek)]

(In reply to comment #2)
> Why are you changing the nsinstall manifest?

not intentional, feel free to ignore it.

Comment 4

[Robert Kaiser]

Comment on attachment 535831 [details] [diff] [review]
v0.5

Looks kinds hackish but I guess we need it if we want to support optional extensions in the installer, which SeaMonkey probably still wants. As SeaMonkey 2.1 needs this in the mozilla-2.0 branch, I'm not sure how this can land anyhow, but I guess it should be there yesterday... ;-(

The packager.mk change looks usable - as I said kinds hackish though.

Comment 5

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 535831 [details] [diff] [review]
v0.5

Ignoring the first hunk, I realize I know this code very little. Lets see what mwu says.

Comment 6

[[e-mail] (:bhearsum)]

Comment on attachment 535831 [details] [diff] [review]
v0.5

I'm not an appropriate reviewer for this.

Comment 7

[Robert Kaiser]

Comment on attachment 535831 [details] [diff] [review]
v0.5

>+  mv core $(MOZ_PKG_DIR) && \
>+  (if [ -d optional ]; then ls -1 optional > ../optional_file_list.txt; mv optional/* $(MOZ_PKG_DIR); fi;)

Hmm, in the SeaMonkey case, the only entry under optional/ should be distribution/ so I wonder if core/ also contains a distribution/ this might fail because mv cannot overwrite an existing distribution/ with a new one. Maybe we should copy here instead (and, if needed to get rid of the original, then just |rm -rf optional|).

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> >+  (if [ -d optional ]; then ls -1 optional > ../optional_file_list.txt; mv optional/* $(MOZ_PKG_DIR); fi;)

I replaced that line with this on the questionable machine:

  (if [ -d optional ]; then ls -1 optional > ../optional_file_list.txt; cp -rp optional/* $(MOZ_PKG_DIR); rm -rf optional; fi;)

...and the before-failing repackage-zip call succeeded on that machine.

I think we should just try and go with that one instead. Callek?

Comment 9

[Robert Kaiser]

Callek gave me an rs+ via email and I deployed http://hg.mozilla.org/releases/mozilla-2.0/rev/ff616f6ced1835adf3a50ad2eda5034f8693b732 (SeaMonkey-specific relbranch). I *think* the outcome of the redone repacks means that this works fine, but I'll let Callek chime in and update the patch here.

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 535831 [details] [diff] [review]
v0.5

>+  (if [ -d optional ]; then ls -1 optional > ../optional_file_list.txt; mv optional/* $(MOZ_PKG_DIR); fi;)

How about |find optional|? It'll leave less junk in the list than ls -l.

Another option would be to use the packaging manifest and MOZ_OPTIONAL_PKG_LIST to identify what belongs in optional, but I don't know if that would work due to whatever we do in packaging. That would be preferred if possible since it would make the question of whether you unpack the installer or the zip not matter anymore. But, I don't feel too strongly either way.

Comment 11

[Justin Wood (:Callek)]

CC-ing joduinn in the hopes that this might be able to land for Firefox5//moz-beta.

Comment 12

[Justin Wood (:Callek)]

Attachment: tested version

This is the tested version we used on SeaMonkey 2.1 and works fine for Gecko 5.

The OPTIONAL method suggested will actually make this code cleaner/better in the end, but I don't want to break what I know works (even though would be nicer) in the short amount of time I have to devote to this, and the timeframe I need to get this in.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 539112 [details] [diff] [review]
tested version

This looks good to me.

Nominating for beta approval for Callek.

Comment 14

[Robert Kaiser]

Comment on attachment 539112 [details] [diff] [review]
tested version

Nominating for aurora as well (Callek, I have tried and it does apply fine). SeaMonkey will need this in one way or another (regular or relbranch) for all builds that have repacks, on all branches.

Comment 15

[LegNeato]

Comment on attachment 539112 [details] [diff] [review]
tested version

Actually, I think we want to let this bake on m-c first before landing on aurora or beta

Comment 16

[Justin Wood (:Callek)]

Landed on m-c for Gecko 7
http://hg.mozilla.org/mozilla-central/rev/54f18b2b93d2

TODO:
* Keep an eye on repacks, make sure that none fail due to a strange edge-case I failed to see/test properly.
* Get approval's triaged.
* Land on branches [either relbranch or default based on approval step]

(marking t-m for SeaMonkey2.1 as this landed on the relbranch for that)

Comment 17

[LegNeato]

Comment on attachment 539112 [details] [diff] [review]
tested version

Approved for aurora, we're still debating on beta

Comment 18

[Johnathan Nightingale [:johnath]]

Too late for FF5 (it goes out tomorrow!)

Comment 19

[Justin Wood (:Callek)]

(In reply to comment #16)
> Landed on m-c for Gecko 7
> http://hg.mozilla.org/mozilla-central/rev/54f18b2b93d2

Backout out on -inbound due to SeaMokey Bug 666518 no longer requiring this.

> TODO:
> * Keep an eye on repacks, make sure that none fail due to a strange
> edge-case I failed to see/test properly.

This was done, and no problems.

> * Get approval's triaged.
> * Land on branches [either relbranch or default based on approval step]

* I got aurora approval, but never landed there, won't now.
* Landed on a relbranch of m-beta for SM2.2b1 [http://hg.mozilla.org/releases/mozilla-beta/rev/ac0dae3c3041] but won't reuse that relbranch, so no need to backout there.

> (marking t-m for SeaMonkey2.1 as this landed on the relbranch for that)

Would mark WONTFIX but this was shipped in 2.1 final, so technically it was fixed.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

backed out on central as well
http://hg.mozilla.org/mozilla-central/rev/74091287d2eb
http://hg.mozilla.org/mozilla-central/rev/4ab18b537543