1366917 - Provide ability to include the system's RAM in the update URL

Status: VERIFIED FIXED

(Toolkit :: Application Update, enhancement)

Description

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

It looks like this will be needed for the Firefox 32 to Firefox 64 migration via app update.

Comment 1

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

Attachment: patch rev1

Pushed to try with a couple of other patches
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27763e929b4c08f4603f775696ca943d3f5a114a

I suspect this value might be added to %SYSTEM_CAPABILITIES% as well (I'll discuss this with bhearsum) but that can be done in a new bug.

Comment 2

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

Ben, what would be best for balrog for this new value? I think it might make sense to add it to %SYSTEM_CAPABILITIES% but I'm fine with adding a new value if that is better. I'd prefer to avoid overriding other values as we have been doing but I'm open to discussion.

Comment 3

[[e-mail] (:bhearsum)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #2)
> Ben, what would be best for balrog for this new value? I think it might make
> sense to add it to %SYSTEM_CAPABILITIES% but I'm fine with adding a new
> value if that is better. I'd prefer to avoid overriding other values as we
> have been doing but I'm open to discussion.

I'll have to tweak Balrog a bit, but I think %SYSTEM_CAPABILITIES% is best. I think it would be a pain if we needed to add new URL parts whenever we add any piece of hardware information to the update url. If I remember correctly, the original idea behind that field was to include hardware information anyways.

Do you have a particular format in mind? Eg: "SSE,4096"? It's important that we can reliably split the field to make sure we're not treating instructionSet as memory or visa versa. It looks like "unknown" will currently take the place if anything goes wrong finding either of them?

Comment 4

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

I'm fine with using a comma as a delimiter. I don't think we'll ever end up with a value that contains commas for %SYSTEM_CAPABILITIES% and if we do we'll just replace it.

Comment 5

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

Attachment: patch rev2

Ben, will this cause a problem if it is landed for Firefox 55?

Comment 6

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #5)
> Created attachment 8870495 [details] [diff] [review]
> patch rev2
> 
> Ben, will this cause a problem if it is landed for Firefox 55?
I think you can ignore both of those fields since clients have them have a compatible CPU instruction set and memory shouldn't be used at this time.

Comment 7

[[e-mail] (:bhearsum)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #6)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #5)
> > Created attachment 8870495 [details] [diff] [review]
> > patch rev2
> > 
> > Ben, will this cause a problem if it is landed for Firefox 55?
> I think you can ignore both of those fields since clients have them have a
> compatible CPU instruction set and memory shouldn't be used at this time.

Good point. I just finished testing though, and the current code copes with commas correctly - we can do multiple matches (SSE3 && 4096) already. If we want to do < or > matching on the memory portion we'll need bug 1367054.

Comment 8

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

Comment on attachment 8870495 [details] [diff] [review]
patch rev2

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=555165e382febd27f161f4441944c26d8eb5ee8d

Comment 9

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

(In reply to Ben Hearsum (:bhearsum) from comment #7)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #6)
> > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> > comment #5)
> > > Created attachment 8870495 [details] [diff] [review]
> > > patch rev2
> > > 
> > > Ben, will this cause a problem if it is landed for Firefox 55?
> > I think you can ignore both of those fields since clients have them have a
> > compatible CPU instruction set and memory shouldn't be used at this time.
> 
> Good point. I just finished testing though, and the current code copes with
> commas correctly - we can do multiple matches (SSE3 && 4096) already. If we
> want to do < or > matching on the memory portion we'll need bug 1367054.
I'm certain this will need to determine if memory is equal to or greater than 4096 so bug 1367054 will be necessary.

Comment 10

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

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8)
> Comment on attachment 8870495 [details] [diff] [review]
> patch rev2
> 
> Pushed to try
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=555165e382febd27f161f4441944c26d8eb5ee8d
Since Mac opt is having trouble I pushed again for just Mac debug
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e7cad2212dfe5407ee3a43be0e096ab7976247b

Comment 11

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

Comment on attachment 8870495 [details] [diff] [review]
patch rev2

The first try push passed and I verified that the build farm is having problems building mac opt. I expect that the second try push for mac debug will pass so I'm requesting review. I also checked with bhearsum regarding whether this is ok to land as soon as this is r+'d and he is fine with it.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a936e628dbb3
Include the system's RAM in the %SYSTEM_CAPABILITIES% value of the update URL. r=mhowell

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a936e628dbb3

Comment 14

[Abe - QA (:Abe_LV)]

QE Verified as Fixed.
Here are test cases and runs: https://public.etherpad-mozilla.org/p/1366917
Needinfo me if you have questions or feedback.