Closed Bug 1402344 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: MFBT, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: tjr, Assigned: Waldo)

References

Details

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

Attachments

(8 files)

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.
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
Attachment #8916869 - Flags: review?(nfroyd)
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)
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+
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
https://hg.mozilla.org/mozilla-central/rev/840478b0ff49
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.