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

VERIFIED FIXED in Firefox 55

Status

()

Toolkit
Application Update
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

It looks like this will be needed for the Firefox 32 to Firefox 64 migration via app update.
Created attachment 8870216 [details] [diff] [review]
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.
Attachment #8870216 - Flags: review?(mhowell)
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.
Flags: needinfo?(bhearsum)
Blocks: 1274659
(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?
Depends on: 1367054
Flags: needinfo?(bhearsum)
Attachment #8870216 - Flags: review?(mhowell)
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.
Created attachment 8870495 [details] [diff] [review]
patch rev2

Ben, will this cause a problem if it is landed for Firefox 55?
Attachment #8870216 - Attachment is obsolete: true
Attachment #8870495 - Flags: feedback?(bhearsum)
(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.
(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.
(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.
(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 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.
Attachment #8870495 - Flags: review?(mhowell)
Attachment #8870495 - Flags: review?(mhowell) → review+

Comment 12

a year ago
Pushed by rstrong@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a936e628dbb3
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.