1119776 - Don't define snprintf if MSVC provides it (VS2015 and later do)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

These patches are required in order for VS2015 to build mozilla-central correctly.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Avoid defining snprintf when MSVC provides it (dom/media)

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Avoid defining snprintf when MSVC provides it (video)

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3: Avoid defining snprintf when MSVC provides it (WebRTC)

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 4: Avoid defining snprintf when MSVC provides it (MFBT)

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 5: Avoid defining snprintf when MSVC provides it (netwerk)

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 6: Avoid defining snprintf when MSVC provides it (NSPR)

Ted, I'll check this into the NSPR repository after it is r+d.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 7: Avoid defining snprintf when MSVC provides it (other)

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 8: Avoid defining snprintf when MSVC provides it (gfx)

Benoit,

I understand that your team upstreams patches to Angle and Skia before landing them in mozilla-central. Could you help me do that for these changes? I think it's likely to be trivial for you, but easy for me to screw up.

Thanks!

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Here's a try run with these patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c63b55fd66a5

I'm not sure what the deal is with Android Mochitest-8. I think maybe I had my local tree based on something that broke that test; I'll verify that before landing any of this.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8546605 [details] [diff] [review]
Part 5: Avoid defining snprintf when MSVC provides it (netwerk)

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

Please send email to Michael Tuexen (Michael Tuexen <[e-mail]>) with a pointer to this patch so he can land it in upstream.  Thanks!

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8546600 [details] [diff] [review]
Part 2: Avoid defining snprintf when MSVC provides it (video)

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

Ralph is more up with the play WRT whether we're taking patches directly against libvpx etc, so I'll pass the review to him.

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8546600 [details] [diff] [review]
Part 2: Avoid defining snprintf when MSVC provides it (video)

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

Please split the libvpx change into a separate commit.

For libvpx, in addition to the change to vp9_systemdependent.h please add a patch file to media/libvpx/ with the same change and add it to apply_patches() in update.py (see https://github.com/mozilla/gecko-dev/blob/master/media/libvpx/update.py#L530). This will ensure your change isn't clobbered the next time we pull from this library's upstream source.

Stagefright patch looks fine. r=me with these changes.

Comment 13

[(not currently active) Ted Mielczarek]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #6)
> Ted, I'll check this into the NSPR repository after it is r+d.

I just tagged a new NSPR beta the other day, feel free to tag another one. Given that today is an uplift day, though, we might want to cut a NSPR release from the current trunk and then land this patch for the next version (just to minimize churn when we land the NSPR release changeset on aurora).

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546608 [details] [diff] [review]
Part 8: Avoid defining snprintf when MSVC provides it (gfx)

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

Jeff, could you help me with the review for this patch and to help merge it upstream? I don't have access to the upstream repositories to do the merge myself. Thanks!

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8546608 [details] [diff] [review]
Part 8: Avoid defining snprintf when MSVC provides it (gfx)

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

I'll do what I can. It may be a bit of a long process.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546599 [details] [diff] [review]
Part 1: Avoid defining snprintf when MSVC provides it (dom/media)

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f852d38c615

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546600 [details] [diff] [review]
Part 2: Avoid defining snprintf when MSVC provides it (video)

https://hg.mozilla.org/integration/mozilla-inbound/rev/60e60567879b

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546601 [details] [diff] [review]
Part 3: Avoid defining snprintf when MSVC provides it (WebRTC)

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca86f8617aa

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546604 [details] [diff] [review]
Part 4: Avoid defining snprintf when MSVC provides it (MFBT)

https://hg.mozilla.org/integration/mozilla-inbound/rev/6979b5854ef4

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546605 [details] [diff] [review]
Part 5: Avoid defining snprintf when MSVC provides it (netwerk)

https://hg.mozilla.org/integration/mozilla-inbound/rev/b44a29a96736

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546606 [details] [diff] [review]
Part 6: Avoid defining snprintf when MSVC provides it (NSPR)

Part 6 was checked into NSPR and then the new version of NSPR was imported into mozilla-inbound/central, so Part 6 isn't needed any more.

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8546607 [details] [diff] [review]
Part 7: Avoid defining snprintf when MSVC provides it (other)

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fbe6908e21f

Comment 23

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> I'll do what I can. It may be a bit of a long process.

Thanks Jeff! I appreciate it.

Also, thanks a lot to everybody else that reviewed these patches!

Comment 24

[Jeff Walden [:Waldo]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #23)

https://twitter.com/sayrer/status/19304989209

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f852d38c615
https://hg.mozilla.org/mozilla-central/rev/60e60567879b
https://hg.mozilla.org/mozilla-central/rev/9ca86f8617aa
https://hg.mozilla.org/mozilla-central/rev/6979b5854ef4
https://hg.mozilla.org/mozilla-central/rev/b44a29a96736
https://hg.mozilla.org/mozilla-central/rev/3fbe6908e21f

Comment 26

[Ralph Giles (:rillian)]

Attachment: Part 2: Aurora backport of stagefright changes.

This is a backport of just the libstagefright changes from part 2 to reduce variance between firefox 37 and 38.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Harder to backport other changes later.
[Describe test coverage new/current, TBPL]: Landed on m-c
[Risks and why]: Minimal. Include changes for MSVC 2015.
[String/UUID change made/needed]: None

Approval is requested for this patch alone.

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8549728 [details] [diff] [review]
Part 2: Aurora backport of stagefright changes.

Aurora+

Comment 28

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8968645dda79

Comment 29

[Ralph Giles (:rillian)]

Set tracking flags to reflect uplift status.

This isn't really 'fixed' on firefox 37. We only uplifted the stagefright changes.

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 8: Avoid defining snprintf when MSVC provides it (gfx) [v2]

Rebased. Any chance of a review of this any time soon? It's been several months now.

Comment 31

[Jeff Muizelaar [:jrmuizel]]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #30)
> Created attachment 8586307 [details] [diff] [review]
> Part 8: Avoid defining snprintf when MSVC provides it (gfx) [v2]
> 
> Rebased. Any chance of a review of this any time soon? It's been several
> months now.

Sorry about that. It's probably worth splitting out this patch for skia, angle and harfbuzz and have them reviewed and upstreamed individually. I haven't had a chance to do this for each of the projects. gw280 can deal with skia patch, jgilbert can do the angle part and Jonathan Kew can do the harfbuzz part.

Comment 32

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 8: Avoid defining snprintf when MSVC provides it (angle)

Jeff, Jeff M. suggested you are the best person to review and upstream this. All the non-gfx patches in this bug have already been checked in. Thanks in advance!

Comment 33

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 9: Avoid defining snprintf when MSVC provides it (harfbuzz)

Jonathan, Jeff M. suggested you are the best person to review and upstream this. All the non-gfx patches in thus bug have already landed. Thanks in advance!

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 10: Avoid defining snprintf when MSVC provides it (skia)

George, Jeff M. suggested you are the best person to review and upstream this. All the non-gfx patches in this bug have already landed. Thanks in advance!

Comment 35

[Behdad Esfahbod]

HarfBuzz patch is upstream now.

Comment 36

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8586317 [details] [diff] [review]
Part 8: Avoid defining snprintf when MSVC provides it (angle)

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

This is already present in upstream ANGLE.

Comment 37

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Behdad Esfahbod from comment #35)
> HarfBuzz patch is upstream now.

Great! Can you r+ the patch so I can land it? Thanks!

Comment 38

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8586317 [details] [diff] [review]
Part 8: Avoid defining snprintf when MSVC provides it (angle)

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c264a5558d6

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c264a5558d6

Comment 40

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8586324 [details] [diff] [review]
Part 10: Avoid defining snprintf when MSVC provides it (skia)

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

I'll upstream this

Comment 41

[George Wright (:gw280) (needinfo me!)]

https://codereview.chromium.org/1051273004

Comment 42

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8586321 [details] [diff] [review]
Part 9: Avoid defining snprintf when MSVC provides it (harfbuzz)

https://hg.mozilla.org/integration/mozilla-inbound/rev/25178d030eef

Comment 43

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8586324 [details] [diff] [review]
Part 10: Avoid defining snprintf when MSVC provides it (skia)

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b20a8b5edec

Comment 44

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks for all the reviews! All the patches for this have landed now.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25178d030eef
https://hg.mozilla.org/mozilla-central/rev/9b20a8b5edec

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Calling Fx39 fixed to make my "needs uplift" bug queries happy (I'm assuming we don't intend to backport any further patches from this bug to the release branches).