Moving large pragma warning block from pybind11.h to a separate header file.

[rwgk]

Fixes widespread MSVC and GCC failures (link below) after iwyu cleanup under PR #2672.

Changelog not needed.

This really bit me, hard, when I was trying to make sense of the error messages, after getting these CI failures:

https://github.com/pybind/pybind11/runs/1965988924

[bstaletic]

If the idea is to disable these warnings completely,

No. The idea is to unblock doing the right thing on the smart_holder branch (including iwyu cleanup).

just do it in cmake. However, shouldn't we fix warnings instead?

Yes and no. Roughly (no opinions, just pros-and-cons):

• Yes, to keep the code clean.
• No, because then you need to duplicate the options for other build tools.

In any case, that's clearly a question for a separate PR. This PR is just slightly shifting the status-quo, in a way that's immediately helpful. A small step to make things a little better, not an attempt to be perfect. If you want it all at once you may wind up just dwelling on highly localized issues.

Note that I am/will be using 2.5 other build tools (blaze/bazel, scons).

[YannickJadoul]

More importantly and crucially, this completely ignores the popping part of the "ignore these warnings":

pybind11/include/pybind11/pybind11.h

Lines 2284 to 2288 in 74a767d

	#if defined(_MSC_VER) && !defined(__INTEL_COMPILER)
	# pragma warning(pop)
	#elif defined(__GNUG__) && !defined(__clang__)
	# pragma GCC diagnostic pop
	#endif

If you push something onto a stack, better pop it as well in the same scope/context/file.

(This really bit me, hard, when I was trying to make sense of the error messages.)

How exactly did it bite you, if I can ask? I suppose this would happen when you don't include pybind11.h as first header?

If the idea is to disable these warnings completely, just do it in cmake. However, shouldn't we fix warnings instead?

Agreed with @bstaletic (except that I'm not sure how ignoring these in CMake would interact with user code; @henryiii will know, once he has time again, back from much more important things than pybind11 :-) ).
Also, in general, the more local we can ignore these warnings, the better, I'd say? So if some warnings should be ignored in only one file, it's better to wrap that file with a push-pop thing. Localized things are better than these global ones in pybind11.h :-)

[rwgk]

@YannickJadoul I updated the PR description to include a link to the CI failures.

[rwgk]

@YannickJadoul asked:

I suppose this would happen when you don't include pybind11.h as first header?

Yes, exactly. See the CI failures. Slightly oversimplified: I'm getting punished for wanting to make the smart_holder code iwyu clean.

I agree it would be nice to clean up the use of pragmas, but I believe it'll be many rounds of CI before we have it clamped down to exactly what's needed. I'm balancing that effort against: the vast majority of extensions I've seen #include "pybind11/pybind11.h" anyway. Practically, very little is won for that effort spent. And there are so many bigger fish to fry; see my suggested roadmap doc: https://docs.google.com/document/d/1NUGUkHYkpylmyFsqnEyARlQvENoXCxDFpsbH6sfAzHg/

Imagine we had the code refactored as suggested there, the pragma cleanup would be easier and higher quality (because the pragmas could be applied more granularly). Doing the pragma cleanup before the refactoring is the wrong order IMO in the big picture.

[henryiii]

I am not in favor of this change. Warnings must be added and popped from the same file. Eventually, a "warn what you use" style would be better, where warnings are removed and popped back just where they are needed (except for a few nonsense warnings that could remain project-wide). Honestly, I bet a few of these could be removed. If someone "IWYU"'s pybind11, this will add warnings from detail/common.h that then never get cleared, masking user code bugs. (We don't really support that, AFAIK, but it highlights why it's wrong to move these).

These are just warnings. If you want to do fast development, don't build with warnings turned into errors - problem solved! If you want to put something in master, no warning suppressions can be allowed to leak, and warnings are turned into errors so we don't spew warnings in user code without failing tests (nobody reads test logs unless there's a red ❌).

[rwgk]

I am not in favor of this change. Warnings must be added and popped from the same file. Eventually, a "warn what you use" style would be better, where warnings are removed and popped back just where they are needed (except for a few nonsense warnings that could remain project-wide). Honestly, I bet a few of these could be removed. If someone "IWYU"'s pybind11, this will add warnings from detail/common.h that then never get cleared, masking user code bugs. (We don't really support that, AFAIK, but it highlights why it's wrong to move these).

These are just warnings. If you want to do fast development, don't build with warnings turned into errors - problem solved! If you want to put something in master, no warning suppressions can be allowed to leak, and warnings are turned into errors so we don't spew warnings in user code without failing tests (nobody reads test logs unless there's a red ❌).

Thanks @henryiii I agree with everything you write and request, but my situation is not such that I can jump on tackling every piece of tech debt that I happen to encounter, like this one.

Looking at this PR in isolation, you're absolutely right, I'd say the same things. But when I created this PR ~6 weeks ago, I was sitting on the 200+ commits mega PR #2672 that in the meantime has become the start of the smart_holder branch. This PR here is currently a tiny part of the bigger picture, an attempt to keep the smart_holder branch and master in sync as much as my resources allow. There is no evidence that this change breaks anything, but it was necessary for the smart_holder work, in large part to meet the IWYU requests related to PR #2841. I understand, if the smart_holder work is not a shared perspective, it makes sense to reject the change here and demand immediate cleanup of things that I happen to stumble over, at the time when I stumble over them. I wish I had infinite resources to do cleanups on the go, but it's really only me and one other person attempting to integrate pybind11 into PyCLIF. Recurring requests for collateral cleanups is not something we can absorb without help or proper planning and prioritization. That's why I put together the suggested roadmap. I didn't get any feedback. — Many times I was thinking of looking for help inside Google, but I always held off because I heard concerns about an apparent Google takeover. So I feel really boxed in.

I will have to plow on with the pybind11 integration into PyCLIF, to see what other major roadblocks I need to work on (the smart_holder work took the better part of 3 months; yesterday we discovered that pybind11 doesn't have support for the tp_as_sequence protocol, which I will need to work on; and there are the nagging doubts about the MI UB, if that bites I don't know where we'll land). With a little luck, I can keep muddling through like I've done so far, keeping the smart_holder branch reasonably compatible with master. I'm holding out hope that it doesn't come to a point where I have to choose between a breakdown of the PyCLIF-pybind11 integration project, or diverging irrecoverably from master. With a little more luck, when PyCLIF is actually using pybind11 in production, I'll be in a much better position to secure the resources for pure cleanup work, even if it drags out over many months (that's what I think have to expect realistically, given the experience of the past 9 months).

I'll change this PR to draft and will keep it around for reference for a while (or until it becomes obsolete). If someone else picks up the cleanup of the warning pragmas in the meantime, I'll do my best to integrate those changes into the smart_holder branch.

[rwgk]

Hi @henryiii, I changed this PR to use a more conservative approach: the pragma block is simply split out into a separate header file, and included in pybind11.h exactly where it used to be, i.e. the net result on master is "no change at all except that the pragma block is in a separate file". Does this look acceptable to you? — I changed the smart_holder branch accordingly in #2988.

[rwgk]

I removed this from the 2.7 milestone list and marked it as draft for now, although it's one of the biggest diffs between master and smart_holder. But it can wait. I didn't have any merge conflicts because of this diff.

[rwgk]

This PR has become obsolete with the work started under PR #3127.

[rwgk]

More importantly and crucially, this completely ignores the popping part of the "ignore these warnings":

I'm sorry @YannickJadoul, I misunderstood this pointer back in February. You are absolutely right.

My 2nd attempt, moving the block to a separate file, also wasn't correct of course in hindsight. Unfortunately it took me way too long (yesterday) to realize where the pop are that you pointed out here (at the bottom of pybind11.h).

For general reference, if someone looks here in the future, I'm in the middle of a 3rd attempt to enable IWYU cleanup and refactoring of header files. For that I built this list of source code locations from where the suppressed warnings originate, plus an overview spreadsheet. Based on that, currently I don't see an easy way to do it correctly.