Update double-conversion to 2017-09-15 00:00:00

RESOLVED FIXED in Firefox 58

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tjr, Assigned: Waldo)

Tracking

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [third-party-lib-audit])

Attachments

(8 attachments)

Reporter

Description

2 years ago
This is a (semi-)automated bug making you aware that there is an available upgrade for an embedded third-party library. You can leave this bug open, and it will be updated if a newer version of the library becomes available. If you close it as WONTFIX, please indicate if you do not wish to receive any future bugs upon new releases of the library.

double-conversion is currently at version 2016-11-23 19:14:38 in mozilla-central, and the latest version of the library released is 2017-09-15 00:00:00. 

I fetched the latest version of the library from https://github.com/google/double-conversion.
Assignee

Comment 1

2 years ago
We're getting too many revs behind for comfort, IMO, so it's time for an update even if nothing too important's changed.  I have patches locally that should update us to latest tip.

In a moderate speedbump, upstream changed all its #includes from "*.h" to <double-conversion/*.h>, so this was somewhat messy -- and I had to hg mv all of mfbt/double-conversion/source to mfbt/double-conversion/double-conversion to make it all work, which is going to muck with easy blame some.  Sest la vee.  A JS shell builds with them, just testing whether Firefox does before I upload the patchwork.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee

Comment 6

2 years ago
Attachment #8916869 - Flags: review?(nfroyd)
Assignee

Comment 9

2 years ago
That media/ change's necessity is super-squirrelly.  I expect that in the (separate, not yet filed) bug where I remove mozilla/double-conversion.h in favor of double-conversion/double-conversion.h directly, I'll inline the stupid bits of utils.h into that header and remove a very dumb dependency.

The JS engine builds locally with this, Firefox builds locally with this.  Will need to try this before pushing, natch.
Attachment #8916873 - Flags: review?(nfroyd)
Assignee

Comment 10

2 years ago
As for the upstream changes this accepts: basically it's various build changes, some fixes to avoid doing negative shifts in some places (which might well fix some bug reports we've gotten, actually -- will need to search those out once this lands), adds a case-insensitive mode we don't need/use, and does other minor things of little importance.  Full list at https://github.com/google/double-conversion from November 23, 2016 to September 15, 2017.  (I'm having trouble finding a way to do arbitrary-rev diffs on Github, boo.)  Nothing seems too momentous, nothing so worrisome that it demands strict scrutiny and our own second reviewing, IMO.
Comment on attachment 8916865 [details] [diff] [review]
1 - Move mfbt/double-conversion/source to mfbt/double-conversion/double-conversion, preparing for an upstream #include-path change

This source directory is starting to look like Python module/module/module layouts. =D
Attachment #8916865 - Flags: review?(nfroyd) → review+
Comment on attachment 8916866 [details] [diff] [review]
2 - Update the double-conversion update script to work with the new source location

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

::: mfbt/double-conversion/update.sh
@@ +12,5 @@
>  
> +#LOCAL_PATCHES="$LOCAL_PATCHES add-mfbt-api-markers.patch"
> +#LOCAL_PATCHES="$LOCAL_PATCHES use-StandardInteger.patch"
> +#LOCAL_PATCHES="$LOCAL_PATCHES use-mozilla-assertions.patch"
> +#LOCAL_PATCHES="$LOCAL_PATCHES ToPrecision-exponential.patch"

I'm going to assume that some of these changes are going to be backed out by future patches.
Attachment #8916866 - Flags: review?(nfroyd) → review+
Attachment #8916867 - Flags: review?(nfroyd) → review+
Comment on attachment 8916868 [details] [diff] [review]
4 - Update add-mfbt-api-markers.patch to apply to latest double-conversion source

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

::: mfbt/double-conversion/add-mfbt-api-markers.patch
@@ +10,5 @@
>   #ifndef DOUBLE_CONVERSION_DOUBLE_CONVERSION_H_
>   #define DOUBLE_CONVERSION_DOUBLE_CONVERSION_H_
>   
>  +#include "mozilla/Types.h"
> + #include <double-conversion/utils.h>

WDYT about using the "" convention for headers?  I think we have an unstated rule that <> is only used for "system" headers, and we use "" for everything else.  Oh, I see that the upstream source apparently uses the <> convention everywhere.  OK, when in Rome, I guess...
Attachment #8916868 - Flags: review?(nfroyd) → review+
Attachment #8916869 - Flags: review?(nfroyd) → review+
Attachment #8916870 - Flags: review?(nfroyd) → review+
Attachment #8916872 - Flags: review?(nfroyd) → review+
Comment on attachment 8916873 [details] [diff] [review]
8 - Make the double-conversion code be correctly built in its new imported location

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

::: mfbt/decimal/moz-decimal-utils.h
@@ +10,4 @@
>  // of Decimal.cpp under the Mozilla source without blink core dependencies. Do
>  // not include it into any file other than Decimal.cpp.
>  
> +#include "../double-conversion/double-conversion/double-conversion.h"

DOUBLE-CONVERSION-CEPTION
Attachment #8916873 - Flags: review?(nfroyd) → review+

Comment 15

2 years ago
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/840478b0ff49
Update mfbt-double-conversion to the latest upstream rev.  r=froydnj

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/840478b0ff49
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.