Make Graphene NSIS installer for Windows

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: graphene-larch [horizon][webvr])

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

3 years ago
This bug is to use NSIS similar to how Firefox desktop does to create an installer for Windows for graphene builds.
(Assignee)

Comment 1

3 years ago
Created attachment 8625516 [details] [diff] [review]
Fork Firefox installer files
Attachment #8625516 - Flags: feedback?(fabrice)
(Assignee)

Comment 2

3 years ago
Created attachment 8625517 [details] [diff] [review]
Graphene Window installer fork changes
Attachment #8625517 - Flags: feedback?(fabrice)
(Assignee)

Comment 3

3 years ago
Created attachment 8625518 [details] [diff] [review]
Graphene installer branding changes for unofficial, official, and horizon
Attachment #8625518 - Flags: feedback?(fabrice)
(Assignee)

Updated

3 years ago
Whiteboard: graphene-larch
(Assignee)

Comment 4

3 years ago
Created attachment 8625526 [details] [diff] [review]
Graphene Window installer fork changes
Attachment #8625517 - Attachment is obsolete: true
Attachment #8625517 - Flags: feedback?(fabrice)
Attachment #8625526 - Flags: feedback?(fabrice)
(Assignee)

Comment 5

3 years ago
Created attachment 8625527 [details] [diff] [review]
Graphene installer branding changes for unofficial, official, and horizon
Attachment #8625518 - Attachment is obsolete: true
Attachment #8625518 - Flags: feedback?(fabrice)
Attachment #8625527 - Flags: feedback?(fabrice)
Attachment #8625516 - Flags: feedback?(fabrice) → feedback+
Attachment #8625526 - Flags: feedback?(fabrice) → feedback+
Attachment #8625527 - Flags: feedback?(fabrice) → feedback+
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?
(Assignee)

Comment 7

3 years ago
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.
Flags: needinfo?(robert.strong.bugs)
I'll take a look at it soon.
Whiteboard: graphene-larch → graphene-larch [horizon][webvr]
(Assignee)

Comment 9

3 years ago
Friendly ping on review
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.
Flags: needinfo?(robert.strong.bugs)
Comment on attachment 8625527 [details] [diff] [review]
Graphene installer branding changes for unofficial, official, and horizon

Everything after !define URLStubDownload is not needed
(Assignee)

Comment 12

3 years ago
> 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?
Could you ifdef the code to determine which you are building for and then use something new other than BrandFullNameInternal?
(Assignee)

Comment 14

3 years ago
OK, I'll do that instead.
Thanks. I think that is better / safer than extending the purpose of BrandFullNameInternal
(Assignee)

Comment 16

3 years ago
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.
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.
(Assignee)

Comment 18

3 years ago
I'll make it so, thanks.
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.
(Assignee)

Comment 20

3 years ago
Created attachment 8688106 [details] [diff] [review]
Graphene Window installer fork changes. rev2

Changes since last time is to use the new define for registration to treat Graphene and Horizon differently.
Attachment #8625526 - Attachment is obsolete: true
Attachment #8688106 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 21

3 years ago
Created attachment 8688107 [details] [diff] [review]
Graphene installer branding changes for unofficial, official, and horizon. rev2
Attachment #8625527 - Attachment is obsolete: true
Attachment #8688107 - Flags: review?(robert.strong.bugs)
(Assignee)

Updated

3 years ago
Attachment #8625516 - Flags: review?(robert.strong.bugs)
I won't have time to review this in any reasonable amount of time
(Assignee)

Comment 23

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

Didn't get qref'ed in
Attachment #8688106 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 24

3 years ago
Created attachment 8688202 [details] [diff] [review]
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.
Attachment #8688106 - Attachment is obsolete: true
Attachment #8688202 - Flags: review?(fabrice)
(Assignee)

Updated

3 years ago
Attachment #8688107 - Flags: review?(robert.strong.bugs) → review?(fabrice)
(Assignee)

Updated

3 years ago
Attachment #8625516 - Flags: review?(robert.strong.bugs) → review?(fabrice)
(Assignee)

Comment 25

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

Comment 26

3 years ago
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.
Flags: needinfo?(jmathies)
Attachment #8625516 - Flags: review?(fabrice) → review?(jmathies)
Attachment #8688107 - Flags: review?(fabrice) → review?(jmathies)
Attachment #8688202 - Flags: review?(fabrice) → review?(jmathies)

Comment 27

3 years ago
(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.
Flags: needinfo?(jmathies)

Comment 28

3 years ago
Do we really want this app registering desktop file and proto associations? What's the use case here?
Flags: needinfo?(netzen)

Updated

3 years ago
Attachment #8625516 - Flags: review?(jmathies) → review+

Updated

3 years ago
Attachment #8688107 - Flags: review?(jmathies) → review+
(Assignee)

Comment 29

3 years ago
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.
Flags: needinfo?(netzen)

Comment 30

3 years ago
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.
Attachment #8688202 - Flags: review?(jmathies) → review+
(Assignee)

Comment 31

3 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.