Closed Bug 1101237 Opened 10 years ago Closed 9 years ago

Remove distribution directory during install

Categories

(Thunderbird :: Installer, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 40.0

People

(Reporter: robert.strong.bugs, Assigned: jsbruner)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1098677 made it so apps need to remove the distribution directory on install instead of using the common macro.
Attached patch Patch rev1Splinter Review
If you could own this bug and get it landed I would appreciate it.
Attachment #8524940 - Flags: review?(josiah)
Sure thing.
Assignee: nobody → josiah
Status: NEW → ASSIGNED
Assignee: josiah → nobody
Didn't mean to remove myself.
Assignee: nobody → josiah
Comment on attachment 8524940 [details] [diff] [review]
Patch rev1

I instead meant to remove myself as the reviewer for three reasons. A) I'm very busy for another week or so. B) I don't actually have a Windows machine, and Windows VM builds take forever. And C) Because I don't have a ton of knowledge of the Windows installer/build stuff.

Joshua is probably a better reviewer for all of those reasons. :)
Attachment #8524940 - Flags: review?(josiah) → review?(Pidgeot18)
Comment on attachment 8524940 [details] [diff] [review]
Patch rev1

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

To be frank, I don't particularly know Windows installer logic at all, so I'm going to delegate review to Mike Hommey or someone else who knows this stuff.
Attachment #8524940 - Flags: review?(Pidgeot18) → review?(mh+mozilla)
You could ask Brian Bondy (bbondy)
Attachment #8524940 - Flags: review?(netzen)
Comment on attachment 8524940 [details] [diff] [review]
Patch rev1

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

::: mail/installer/windows/nsis/installer.nsi
@@ +228,5 @@
> +  ${EndUnless}
> +
> +  ${If} ${FileExists} "$INSTDIR\distribution"
> +  ${AndIf} $0 != "false"
> +    RmDir /r "$INSTDIR\distribution"

I think it's safer here to check for equaling a specific value? like == "true".
Attachment #8524940 - Flags: review?(netzen)
The removal is preferred over keeping it on pave over install.
Comment on attachment 8524940 [details] [diff] [review]
Patch rev1

I'd rather someone who already knows nsi look at this.
Attachment #8524940 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #9)
> Comment on attachment 8524940 [details] [diff] [review]
> Patch rev1
> 
> I'd rather someone who already knows nsi look at this.

Who would that be? Splinter's "Suggested Reviewers" is not very helpful.
Flags: needinfo?(mh+mozilla)
(In reply to Josiah Bruner [:JosiahOne] (needinfo! - Won't respond to CC)) from comment #10)
> (In reply to Mike Hommey [:glandium] from comment #9)
> > Comment on attachment 8524940 [details] [diff] [review]
> > Patch rev1
> > 
> > I'd rather someone who already knows nsi look at this.
> 
> Who would that be? Splinter's "Suggested Reviewers" is not very helpful.

cf. comment 6?
Flags: needinfo?(mh+mozilla)
Attachment #8524940 - Flags: review?(netzen)
Comment on attachment 8524940 [details] [diff] [review]
Patch rev1

See comment 7
Attachment #8524940 - Flags: review?(netzen)
Attached patch Patch (Rev 2) (obsolete) — Splinter Review
Whoops, sorry. Made that change.
Attachment #8524940 - Attachment is obsolete: true
Attachment #8598081 - Flags: review?(netzen)
Try build verifying that it doesn't break builds or tests: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c8e206ff0e9f
Comment on attachment 8598081 [details] [diff] [review]
Patch (Rev 2)

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

Thanks, I think there's one more change which I didn't notice before.

::: mail/installer/windows/nsis/installer.nsi
@@ +226,5 @@
> +      ReadINIStr $0 $1 "Install" "RemoveDistributionDir"
> +    ${EndIf}
> +  ${EndUnless}
> +
> +  ${If} ${FileExists} "$INSTDIR\distribution"

I think this block should go inside the Unless/EndUnless.
Attachment #8598081 - Flags: review?(netzen)
Brian, just to verify that this will do the right thing... the distribution directory is supposed to be removed by default. This way if a person installs a partner build and then installs a non partner build the customizations are removed. So, if there isn't an ini file it should be removed and if there is an ini file it should be removed unless the ini specifies false
Comment on attachment 8524940 [details] [diff] [review]
Patch rev1

Ah I thought we only wanted to do it if the INI was specified. OK then the original patch was OK. Thanks for clarifying Robert. Un-obsoleting the first patch and I'll obsolete the 2nd patch.
Attachment #8524940 - Attachment is obsolete: false
Attachment #8524940 - Flags: review+
Attachment #8598081 - Attachment is obsolete: true
Thanks guys!

Fixed -> https://hg.mozilla.org/comm-central/rev/99d4ab3b8b6f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
You need to log in before you can comment on or make changes to this bug.