1332797 - Update our double-conversion import

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

It's apparently a bit old, and bug 1325771 wants a fix that upstream landed a bit ago.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Make the double-conversion update script clone the double-conversion repository into a temporary directory, then copy out of it, taking an optional git revision to use

A few preliminary cleanupy patches first.

After this patch, |./update.sh 04cae7a8d5ef3d62ceffb03cdc3d38f258457a52| is a no-op.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Clear out the current double-conversion copy before updating it so that our copying properly handles added/removed files

(I actually quasi-undo this momentarily in favor of moving all imported files into a source/ subdirectory, so feel free not to examine this super-closely.)

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Move mfbt/double-conversion source files into a new source/ subdirectory, to segregate upstream files from update.sh and our local patches

The double-conversion source files were manually |hg mv|'d, so revision history shouldn't change for them.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Adjust update.sh to work against tip double-conversion

I haven't fully examined all the revs we'll pick up doing this yet, but this command shows them:

git log 04cae7a8d5ef3d62ceffb03cdc3d38f258457a52..d8d4e668ee1e6e10b728f0671a89b07d7c4d45be

Probably we're good to do this, but I figure I can get this out there in parallel with examining that list.

After this patch, running

  ./update.sh d8d4e668ee1e6e10b728f0671a89b07d7c4d45be

will correctly update our tree.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Update use-mozilla-assertions.patch to work against tip

...or no, it won't quite yet.  With this patch in place, to adjust existing local patches as needed, it will.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Update our various local patches to apply against tip double-conversion

Turns out a patch that has fuzz applied, will make at least some |patch| (like mine) spew an *.orig file.  We wouldn't want to add that, so the patch to always use <stdint.h> needs a no-op refresh.  (This seems preferable to passing the option to make |patch| not spew a *.orig file, which may not be fully portable.)

I'll add some code in a separate patch to make the update script fail if a *.{orig,rej} file is in the tree after it's run.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Make update.sh fail if applying a local patch creates a *.{orig,rej} file

With this patch applied, trying to update fails, properly hitting the error message.  (I'm removing the change to use-StandardInteger.patch locally now -- just wanted to have it there in case you wanted to test this at all yourself.)

Comment 8

[Jeff Walden [:Waldo]]

Attachment: change-graph.txt

I skimmed through all the change summaries this update would pick up.  Nothing jumps out as potentially worrisome.  We could also skim the actual patches.  It seems unlikely to be worth the time/effort, so I haven't done so -- but I could if desired.

Unrelatedly, attachment 8829733 [details] [diff] [review] required a couple mini-fixes to make double-conversion's UNREACHABLE() and UNIMPLEMENTED() macros expand to MOZ_CRASH(), not (MOZ_CRASH()) -- MOZ_CRASH is a do {} while (0), so parenthesizing it is Bad.  Fixed locally, not gonna generate bugspam for an updated patch for it.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Note the double-conversion git revision last used when importing double-conversion code

The comment in update.sh is going to go stale; recording this in a separate file seems much better, and it's what we do for ICU and tzdata.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Delete the temporary directory used to store the double-conversion clone when all's said and done

Not a big deal when a clone is ~26MB, but especially if you're hacking on this stuff, stray clones can add up.  (|du -hs /tmp/| was 1.1GB for me just now, but only 30MB once I cleared out the clones I'd accumulated.)

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8829667 [details] [diff] [review]
Make the double-conversion update script clone the double-conversion repository into a temporary directory, then copy out of it, taking an optional git revision to use

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

I don't really get the -Wshadow patch, but OK.

::: mfbt/double-conversion/fix-Wshadow-issues.patch
@@ +31,5 @@
> +     defined(__sparc__) || defined(__sparc) || defined(__s390__) || \
> +     defined(__SH4__) || defined(__alpha__) || \
> +     defined(_MIPS_ARCH_MIPS32R2) || \
> +-    defined(_AARCH64EL_)
> ++    defined(__AARCH64EL__) || defined(__aarch64__)

This is fixing -Wshadow issues?

::: mfbt/double-conversion/update.sh
@@ +36,5 @@
>  patch -p3 < use-StandardInteger.patch
>  patch -p3 < use-mozilla-assertions.patch
>  patch -p3 < use-static_assert.patch
>  patch -p3 < ToPrecision-exponential.patch
> +patch -p3 < fix-Wshadow-issues.patch

+points for splitting all these patches apart; -points for sliding this one in on the side. :p

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8829668 [details] [diff] [review]
Clear out the current double-conversion copy before updating it so that our copying properly handles added/removed files

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

::: mfbt/double-conversion/update.sh
@@ +28,5 @@
>    git -C "$LOCAL_CLONE" checkout "$1"
>  fi
>  
> +# First clear out everything already present.
> +rm -rf ./*

Maybe check that we're in the directory we think we're supposed to be as a sanity check, so we don't go removing files willy-nilly?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/552a1604e5ef
Make the double-conversion update script clone the double-conversion repository into a temporary directory, then copy out of it, taking an optional git revision to use.  After this patch, |./update.sh 04cae7a8d5ef3d62ceffb03cdc3d38f258457a52| is a no-op.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/de8d389998d3
Clear out the current double-conversion copy before updating it so that our copying properly handles added/removed files.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/89b895c219f2
Move mfbt/double-conversion source files into a new source/ subdirectory, to segregate upstream files from update.sh and our local patches.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/3862719c38c5
Adjust update.sh to work against tip double-conversion.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/95241d956221
Update our various local patches to apply against tip double-conversion.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f0f124580c
Make update.sh fail if applying a local patch creates a *.{orig,rej} file.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d6011de195
Update our double-conversion import using the command |./update.sh d8d4e668ee1e6e10b728f0671a89b07d7c4d45be|.  r=generating-script-was-reviewed
https://hg.mozilla.org/integration/mozilla-inbound/rev/52424aa83fa9
Note the double-conversion git revision last used when importing double-conversion code.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/def2e655ff13
Delete the temporary directory used to store the double-conversion clone when all's said and done.  r=froydnj

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/552a1604e5ef
https://hg.mozilla.org/mozilla-central/rev/de8d389998d3
https://hg.mozilla.org/mozilla-central/rev/89b895c219f2
https://hg.mozilla.org/mozilla-central/rev/3862719c38c5
https://hg.mozilla.org/mozilla-central/rev/95241d956221
https://hg.mozilla.org/mozilla-central/rev/e0f0f124580c
https://hg.mozilla.org/mozilla-central/rev/b8d6011de195
https://hg.mozilla.org/mozilla-central/rev/52424aa83fa9
https://hg.mozilla.org/mozilla-central/rev/def2e655ff13