Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1770093002:
Avoid illegal definition of snprintf in VS 2015 (Closed)
|
Created:
4 years, 9 months ago by brucedawson Modified:
4 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid illegal definition of snprintf in VS 2015
VS 2015 supplies an implementation of snprintf which makes definining
it to _snprintf illegal - previously it was just a really bad idea.
This reinstates the compiler version check so that libxml will compile
in Chromium.
README.chromium was also updated to make it explicit that these checks
need to be added (for now) when config.h is copied.
A corresponding change will need to be made upstream.
BUG=440500, 591920
Committed: https://crrev.com/f7bd300e8c52d6ebb7575a28d4cd9d3bdccaf439
Cr-Commit-Position: refs/heads/master@{#379755}
Patch Set 1 #Patch Set 2 : Fixed typo #
Total comments: 2
Patch Set 3 : Fixed README.chromium #Patch Set 4 : Tweaked #ifdefs #Messages
Total messages: 27 (8 generated)
brucedawson@chromium.org changed reviewers: + dominicc@chromium.org
The recent roll of libxml broke VS 2015 builds of Chromium. This fixes that and updates the instructions. I'll then try making an upstream change.
What's worrisome about this change is that we went from using what amounted to the contents of include/win32config.h before the roll to win32/VC10/config.h after, but this CL does not restore us to include/win32config.h; instead it results in a config file that doesn't match either of those. The real issue here, to me, is that upstream even has both those files to begin with, and they differ in such small ways. I would imagine the right fix is to eliminate one of the two from upstream entirely, figuring out what to do with the small #define differences in the process: HAVE_STDINT_H should probably only be defined for reasonably high values of _MSC_VER, and SEND_ARG2_CAST and GETHOSTBYNAME_ARG_CAST should be #defined as nothing (not #defining them only works for us because we don't build nano[ftp,http].c, I think).
True that. I was focused on making the minimal change from the current point, but it is safer to do the minimal change from where we were last week. I'll tweak the change after lunch.
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
https://codereview.chromium.org/1770093002/diff/20001/third_party/libxslt/REA... File third_party/libxslt/README.chromium (right): https://codereview.chromium.org/1770093002/diff/20001/third_party/libxslt/REA... third_party/libxslt/README.chromium:37: Patch win32/config.h to add "#if _MSC_VER < 1900 // Cannot define this in VS 2015 and above!" This should be in the libxml readme, not the libxslt one?
On 2016/03/07 19:54:31, brucedawson wrote: > True that. I was focused on making the minimal change from the current point, > but it is safer to do the minimal change from where we were last week. > > I'll tweak the change after lunch. I don't have any particular affinity for a given config file. The config chosen for 2.9.2 was completely arbitrary anyway.
Okay, should be all good now. I followed the instructions in README.chromium which means I copied from VC10\config.h (and then edited it). That is what we have done for most of the lifetime of libxml2 in Chromium - the last eight months were an aberration. I updated *which* README.chromium I edited. It is amusing that the presubmit checks warns if you don't update REAMDE.chromium, but it doesn't care which one you update. I also improved the text in README.chromium, and I'll try to land an upstream change to vc10\config.h. https://codereview.chromium.org/1770093002/diff/20001/third_party/libxslt/REA... File third_party/libxslt/README.chromium (right): https://codereview.chromium.org/1770093002/diff/20001/third_party/libxslt/REA... third_party/libxslt/README.chromium:37: Patch win32/config.h to add "#if _MSC_VER < 1900 // Cannot define this in VS 2015 and above!" On 2016/03/07 20:32:36, scottmg (afk mar2-4) wrote: > This should be in the libxml readme, not the libxslt one? D'oh! Done.
Description was changed from ========== Avoid illegal definition of snprintf in VS 2015 VS 2015 supplies an implementation of snprintf which makes definining it to _snprintf illegal - previously it was just a really bad idea. This reinstates the compiler version check so that libxml will compile in Chromium. A corresponding change will need to be made upstream. BUG=440500,591920 ========== to ========== Avoid illegal definition of snprintf in VS 2015 VS 2015 supplies an implementation of snprintf which makes definining it to _snprintf illegal - previously it was just a really bad idea. This reinstates the compiler version check so that libxml will compile in Chromium. README.chromium was also updated to make it explicit that these checks need to be added (for now) when config.h is copied. A corresponding change will need to be made upstream. BUG=440500,591920 ==========
On 2016/03/07 21:17:07, brucedawson wrote: > I also improved the text in README.chromium, and I'll try to land an upstream > change to vc10\config.h. If you don't end up making my arguments above to upstream (about removing one of these two configs and making the other reflect the correct union of parts of both), then let me know where your proposal thread is so I can make the arguments :)
> The real issue here, to me, is that upstream even has both those files > to begin with, and they differ in such small ways. One is checked in, and the other is generated. We have traditionally used the checked-in one, and this change returns us to doing that.
On 2016/03/07 21:22:28, brucedawson wrote: > > The real issue here, to me, is that upstream even has both those files > > to begin with, and they differ in such small ways. > > One is checked in, and the other is generated. We have traditionally used > the checked-in one, and this change returns us to doing that. Both are in the upstream source tree as checked in files: https://git.gnome.org/browse/libxml2/tree/include/win32config.h https://git.gnome.org/browse/libxml2/tree/win32/VC10/config.h Unless this isn't the correct git repo?
On 2016/03/07 21:22:28, brucedawson wrote: > > The real issue here, to me, is that upstream even has both those files > > to begin with, and they differ in such small ways. > > One is checked in, and the other is generated. We have traditionally used > the checked-in one, and this change returns us to doing that. Also, this change does not return us to using either one of these files, since it means we have the top portion of win32/VC10/config.h combined with the bottom portion of include/win32config.h.
Got it. I was looking for config.h instead of *config.h so I didn't see the other one. I've asked the XML mailing list for advice.
> I've asked the XML mailing list for advice. I'd like to land some variant of this change today because otherwise a lot of VS 2015 work is blocked. I don't have strong feelings as to whether it should be the current version or a version that copies include\win32config.h (with appropriate updates to README.chromium). If the xml mailing list comes down with the opposite decision then it's no big deal to change it - none of the other defines seem to make any difference. Thoughts?
I think you should land this. My email to upstream has more info, but in short, I think the ultimate right answer is to copy win32config.h; however, I also think that should be fixed to add the HAVE_STDINT_H define. This patch gets us close to that; the remaining missing defines don't affect Chromium builds.
lgtm
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1770093002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770093002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by pkasting@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1770093002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770093002/60001
Message was sent while issue was closed.
Description was changed from ========== Avoid illegal definition of snprintf in VS 2015 VS 2015 supplies an implementation of snprintf which makes definining it to _snprintf illegal - previously it was just a really bad idea. This reinstates the compiler version check so that libxml will compile in Chromium. README.chromium was also updated to make it explicit that these checks need to be added (for now) when config.h is copied. A corresponding change will need to be made upstream. BUG=440500,591920 ========== to ========== Avoid illegal definition of snprintf in VS 2015 VS 2015 supplies an implementation of snprintf which makes definining it to _snprintf illegal - previously it was just a really bad idea. This reinstates the compiler version check so that libxml will compile in Chromium. README.chromium was also updated to make it explicit that these checks need to be added (for now) when config.h is copied. A corresponding change will need to be made upstream. BUG=440500,591920 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Avoid illegal definition of snprintf in VS 2015 VS 2015 supplies an implementation of snprintf which makes definining it to _snprintf illegal - previously it was just a really bad idea. This reinstates the compiler version check so that libxml will compile in Chromium. README.chromium was also updated to make it explicit that these checks need to be added (for now) when config.h is copied. A corresponding change will need to be made upstream. BUG=440500,591920 ========== to ========== Avoid illegal definition of snprintf in VS 2015 VS 2015 supplies an implementation of snprintf which makes definining it to _snprintf illegal - previously it was just a really bad idea. This reinstates the compiler version check so that libxml will compile in Chromium. README.chromium was also updated to make it explicit that these checks need to be added (for now) when config.h is copied. A corresponding change will need to be made upstream. BUG=440500,591920 Committed: https://crrev.com/f7bd300e8c52d6ebb7575a28d4cd9d3bdccaf439 Cr-Commit-Position: refs/heads/master@{#379755} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f7bd300e8c52d6ebb7575a28d4cd9d3bdccaf439 Cr-Commit-Position: refs/heads/master@{#379755} |
