Closed Bug 1332797 Opened 7 years ago Closed 7 years ago

Update our double-conversion import

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(9 files, 1 obsolete file)

5.12 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.10 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
17.31 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.03 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
8.44 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.74 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
21.81 KB, text/plain
Details
2.22 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
760 bytes, patch
froydnj
: review+
Details | Diff | Splinter Review
It's apparently a bit old, and bug 1325771 wants a fix that upstream landed a bit ago.
A few preliminary cleanupy patches first.

After this patch, |./update.sh 04cae7a8d5ef3d62ceffb03cdc3d38f258457a52| is a no-op.
Attachment #8829667 - Flags: review?(nfroyd)
(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.)
Attachment #8829668 - Flags: review?(nfroyd)
The double-conversion source files were manually |hg mv|'d, so revision history shouldn't change for them.
Attachment #8829670 - Flags: review?(nfroyd)
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.
Attachment #8829671 - Flags: review?(nfroyd)
...or no, it won't quite yet.  With this patch in place, to adjust existing local patches as needed, it will.
Attachment #8829672 - Flags: review?(nfroyd)
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.
Attachment #8829733 - Flags: review?(nfroyd)
Attachment #8829672 - Attachment is obsolete: true
Attachment #8829672 - Flags: review?(nfroyd)
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.)
Attachment #8829741 - Flags: review?(nfroyd)
Attached file 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.
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.
Attachment #8830017 - Flags: review?(nfroyd)
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.)
Attachment #8830562 - Flags: review?(nfroyd)
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
Attachment #8829667 - Flags: review?(nfroyd) → review+
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?
Attachment #8829668 - Flags: review?(nfroyd) → review+
Attachment #8829670 - Flags: review?(nfroyd) → review+
Attachment #8829671 - Flags: review?(nfroyd) → review+
Attachment #8829733 - Flags: review?(nfroyd) → review+
Attachment #8829741 - Flags: review?(nfroyd) → review+
Attachment #8830017 - Flags: review?(nfroyd) → review+
Attachment #8830562 - Flags: review?(nfroyd) → review+
Pushed by jwalden@mit.edu:
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
You need to log in before you can comment on or make changes to this bug.