1176553 - Make Graphene NSIS installer for Windows

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Runtime, defect)

Description

[Brian R. Bondy [:bbondy]]

This bug is to use NSIS similar to how Firefox desktop does to create an installer for Windows for graphene builds.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Fork Firefox installer files

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Graphene Window installer fork changes

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Graphene installer branding changes for unofficial, official, and horizon

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Graphene Window installer fork changes

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Graphene installer branding changes for unofficial, official, and horizon

Comment 6

[[:fabrice] Fabrice Desré]

This all look sensible to me but I'm far from knowing this part of the codebase well. Can we get someone that knows this code take a look even if we're landing on a custom repo?

Comment 7

[Brian R. Bondy [:bbondy]]

Hi Robert,

As per Comment #6 and the previous heads up I messaged you about last week.  Would you mind taking a look? The main goal here is just to have something in place to work off of.  It's landing on a custom tree (larch).  It's an installer for the Graphene runtime which powers browser.html and the VR browser.

Comment 8

[Robert Strong (they/them - no direct email)]

I'll take a look at it soon.

Comment 9

[Brian R. Bondy [:bbondy]]

Friendly ping on review

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 8625516 [details] [diff] [review]
Fork Firefox installer files

Why did you go with BrandFullNameInternal instead of Graphene for so much of this code? These values are difficult to clean up if they are changed intentionally or accidentally (e.g. someone accidentally changes the value) and should be the same across all versions of the app which is why they are hardcoded for things like FirefoxURL, etc.

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 8625527 [details] [diff] [review]
Graphene installer branding changes for unofficial, official, and horizon

Everything after !define URLStubDownload is not needed

Comment 12

[Brian R. Bondy [:bbondy]]

> Why did you go with BrandFullNameInternal instead of Graphene for so much of this code?

I want browser.html and Horizon code treated separately.  Is there a more stable setting per product (browser.html / horizon) that you recommend using?

Comment 13

[Robert Strong (they/them - no direct email)]

Could you ifdef the code to determine which you are building for and then use something new other than BrandFullNameInternal?

Comment 14

[Brian R. Bondy [:bbondy]]

OK, I'll do that instead.

Comment 15

[Robert Strong (they/them - no direct email)]

Thanks. I think that is better / safer than extending the purpose of BrandFullNameInternal

Comment 16

[Brian R. Bondy [:bbondy]]

Do you mind if I put a new value here: https://dxr.mozilla.org/mozilla-central/source/browser/branding/official/branding.nsi#11 (but it would be in the b2g branding only not browser)

Something like !define InstallerBaseInternal with a comment that the value should not change.

I prefer it to ifdefs in case there's ever another branding added we won't need an additional ifdef.

Comment 17

[Robert Strong (they/them - no direct email)]

That's fine but don't base the name off of the existing name for BrandFullNameInternal. The "Internal" suffix for that was only added to differentiate it from BrandFullName.

Comment 18

[Brian R. Bondy [:bbondy]]

I'll make it so, thanks.

Comment 19

[Robert Strong (they/them - no direct email)]

Since this is a new installer it would be a good thing to not set HKCU\Software\Clients\APPNAME.EXE. I haven't had time to clean that up for Firefox yet and would hate to see these get added only to have to clean them up.

Comment 20

[Brian R. Bondy [:bbondy]]

Attachment: Graphene Window installer fork changes. rev2

Changes since last time is to use the new define for registration to treat Graphene and Horizon differently.

Comment 21

[Brian R. Bondy [:bbondy]]

Attachment: Graphene installer branding changes for unofficial, official, and horizon. rev2

Comment 22

[Robert Strong (they/them - no direct email)]

I won't have time to review this in any reasonable amount of time

Comment 23

[Brian R. Bondy [:bbondy]]

Comment on attachment 8688106 [details] [diff] [review]
Graphene Window installer fork changes. rev2

Didn't get qref'ed in

Comment 24

[Brian R. Bondy [:bbondy]]

Attachment: Graphene Window installer fork changes. rev3

Fabrice, you previously feedback+'ed, rstrong did a pass on it which I did review comments for. He doesn't have time to look into it in a reasonable amount of time though per his comment #22.  I'd like to get this landed so Graphene has an installer. I'll maintain to keep parity of the installer whenever rstrong cc's me on installer changes he does on Firefox to keep parity with it.

Comment 25

[Brian R. Bondy [:bbondy]]

@rstrong. Thanks for your honesty in Comment #22. Much better than it sitting here for a long time :)

Comment 26

[Brian R. Bondy [:bbondy]]

Hi Jimm,

Are you able to sanity check the NSIS code in the patches? I talked to Fabrice and he said he'd be OK with r+ing the installer patches but he isn't familiar with NSIS, he would like someone to sanity check it.

rstrong previously reviewed and mentioned a couple of review comments, the new patches include the new comments.  rstrong also mentioned about `HKCU\Software\Clients\APPNAME.EXE.` removal, but we discussed on irc that I'd mirror a fix for that once he does it in Firefox's installer and he CCs me on it.

Comment 27

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #26)
> Hi Jimm,
> 
> Are you able to sanity check the NSIS code in the patches? I talked to
> Fabrice and he said he'd be OK with r+ing the installer patches but he isn't
> familiar with NSIS, he would like someone to sanity check it.

sure.

Comment 28

[Jim Mathies [:jimm]]

Do we really want this app registering desktop file and proto associations? What's the use case here?

Comment 29

[Brian R. Bondy [:bbondy]]

Thanks for the reviews Jim!

> Do we really want this app registering desktop file and proto associations? What's the use case here?

Yep I think so, it will allow the browser.html and Horizon (VR) desktop browsers to handle them on Windows.

Comment 30

[Jim Mathies [:jimm]]

Comment on attachment 8688202 [details] [diff] [review]
Graphene Window installer fork changes. rev3

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

Searched through looking for anything that still seemed specific to Firefox, looks like you got all of that. I did see a few routines in here we don't need for a new browser.

::: b2g/installer/windows/nsis/shared.nsh
@@ +720,5 @@
>    ${Unless} ${Errors}
>      DeleteRegValue HKLM "$0\Capabilities\URLAssociations" "gopher"
>    ${EndUnless}
>  
> +  ; Delete gopher from the user's UrlAssociations if it points to ${InstallerBase}URL.

technically I think you can remove this stuff since it'll never get set for this browser.

@@ +855,5 @@
>        ; remove metro browser splash image data
>        DeleteRegKey HKU "$1\Local Settings\Software\Microsoft\Windows\CurrentVersion\AppModel\SystemAppData\DefaultBrowser_NOPUBLISHERID\SplashScreen\DefaultBrowser_NOPUBLISHERID!${APP_USER_MODEL_ID}"
>      ${Else}
>        ; misc. Metro keys
> +      DeleteRegValue HKU "$1\Software\Mozilla\${InstallerBase}" "CEHDump"

Pretty sure we can remove this entire ${Do} block here. There's more DelegateExecute below this than can go as well.

@@ +891,5 @@
>  !define RemoveDEHRegistration "!insertmacro RemoveDEHRegistration"
>  !define un.RemoveDEHRegistration "!insertmacro RemoveDEHRegistration"
>  
>  ; Removes various directories and files for reasons noted below.
>  !macro RemoveDeprecatedFiles

Looks like this helper can go, there's nothing in here you need to do.

Comment 31

[Brian R. Bondy [:bbondy]]

Thanks for your time on this but I won't be pursuing this bug anymore. If anyone else wants to they can pick it up and re-open later.