[BUG]: strdup cannot be found on Cygwin

QuLogic commented 9 months ago

Required prerequisites

[X] Make sure you've read the documentation. Your issue may be addressed there.
[X] Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
[ ] Consider asking first in the Gitter chat room or in a Discussion.

What version (or hash if on master) of pybind11 are you using?

2.11.1

Problem description

On our test builds with Cygwin, some newly added pybind11 code is no longer compiling due to strdup:

  ccache c++ -Isrc/_c_internal_utils.cpython-39-x86_64-cygwin.dll.p -Isrc -I../../src -I/usr/include/python3.9 -I/usr/local/lib/python3.9/site-packages/pybind11/include -fvisibility=hidden -fvisibility-inlines-hidden -flto=auto -fdiagnostics-color=always -DNDEBUG -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -std=c++17 -O3 -DPY_ARRAY_UNIQUE_SYMBOL=MPL__c_internal_utils_ARRAY_API -MD -MQ src/_c_internal_utils.cpython-39-x86_64-cygwin.dll.p/_c_internal_utils.cpp.o -MF src/_c_internal_utils.cpython-39-x86_64-cygwin.dll.p/_c_internal_utils.cpp.o.d -o src/_c_internal_utils.cpython-39-x86_64-cygwin.dll.p/_c_internal_utils.cpp.o -c ../../src/_c_internal_utils.cpp
  In file included from ../../src/_c_internal_utils.cpp:9:
  /usr/local/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h: In member function 'char* pybind11::cpp_function::strdup_guard::operator()(const char*)':
  /usr/local/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h:331:23: error: 'strdup' was not declared in this scope; did you mean 'strcmp'?
    331 |             auto *t = PYBIND11_COMPAT_STRDUP(s);
        |                       ^~~~~~
        |                       strcmp
  In file included from ../../src/_c_internal_utils.cpp:9:
  /usr/local/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h: In member function 'void pybind11::cpp_function::initialize_generic(pybind11::cpp_function::unique_function_record&&, const char*, const std::type_info* const*, pybind11::size_t)':
  /usr/local/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h:617:46: error: 'strdup' was not declared in this scope; did you mean 'strcmp'?
    617 |             = signatures.empty() ? nullptr : PYBIND11_COMPAT_STRDUP(signatures.c_str());
        |                                              ^~~~~~
        |                                              strcmp
  In file included from ../../src/_c_internal_utils.cpp:9:
  /usr/local/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h: In member function 'pybind11::class_<type_, options>& pybind11::class_<type_, options>::def_property_static(const char*, const pybind11::cpp_function&, const pybind11::cpp_function&, const Extra& ...)':
  /usr/local/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h:1789:33: error: there are no arguments to 'strdup' that depend on a template parameter, so a declaration of 'strdup' must be available [-fpermissive]
   1789 |                 rec_fget->doc = PYBIND11_COMPAT_STRDUP(rec_fget->doc);
        |                                 ^~~~~~
  /usr/local/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h:1789:33: note: (if you use '-fpermissive', G++ will accept your code, but allowing the use of an undeclared name is deprecated)
  /usr/local/lib/python3.9/site-packages/pybind11/include/pybind11/pybind11.h:1797:33: error: there are no arguments to 'strdup' that depend on a template parameter, so a declaration of 'strdup' must be available [-fpermissive]
   1797 |                 rec_fset->doc = PYBIND11_COMPAT_STRDUP(rec_fset->doc);
        |                                 ^~~~~~

Unfortunately, the compiler backtrace only seems to point to the #include line, so I'm sure what exact code triggers this.

Reproducible example code

No response

Is this a regression? Put the last known working version here if it is.

Not a regression

ksunden commented 8 months ago

https://github.com/pybind/pybind11/blob/39e65e10d05bfdc80413a6290bdae3391f0f93d7/include/pybind11/pybind11.h#L116-L120

My best guess (with no windows box handy to test it on) is that Cygwin also requires the _strdup version, but does not define _MSC_VER, and so that condition may be able to be expanded to e.g. #if defined(_MSC_VER) || defined(__CYGWIN__) (got the __CYGWIN__ based off of another part of the codebase).

The other option would be the inverse, that it is getting _strdup and needs strdup (and thus && !), but based on the error message not having an _, I think the former is more likely.

DWesl commented 7 months ago

Cygwin /usr/include/string.h:

#if __MISC_VISIBLE || __POSIX_VISIBLE >= 200809 || __XSI_VISIBLE >= 4
char    *strdup (const char *) __malloc_like __result_use_check;
#endif

Cygwin /usr/include/python3.9/pyconfig.h:

/* Define to activate features from IEEE Stds 1003.1-2008 */
#define _POSIX_C_SOURCE 200809L

Maybe a #include <Python.h> is missing somewhere? Per the python C-API documentation:

Since Python may define some pre-processor definitions which affect the standard headers on some systems, you must include Python.h before any standard headers are included.

Given the first pybind11 example in the documentation is

#include <pybind11/pybind11.h>

int add(int i, int j) {
    return i + j;
}

PYBIND11_MODULE(example, m) {
    m.doc() = "pybind11 example plugin"; // optional module docstring

    m.def("add", &add, "A function that adds two numbers");
}

either the docs need to change to have #include <Python.h> before the pybind11 include, or include/pybind11/pybind11.h needs to have `#include on line 13 or so.

This was the cause of scipy/scipy#17925, which was fixed in scipy/scipy#18049

DWesl commented 7 months ago

Chasing includes: the first include in pybind11/pybind11.h is detail/class.h, which first includes ../attr.h, which in turn first includes detail/common.h https://github.com/pybind/pybind11/blob/8b48ff878c168b51fe5ef7b8c728815b9e1a9857/include/pybind11/detail/common.h#L226-L233 are the two actual included files before Python.h on line 274. This does not seem to be a problem on the pybind side.

On the other hand, matplotlib's src/_c_internal_utils.cpp opens with #include <stdexcept>. matplotlib/matplotlib#27821 describes one way to fix this; alternately, moving the stdexcept include after the pybind11 include should also work.

DWesl commented 6 months ago

Closed by matplotlib/matplotlib#27821?

QuLogic commented 6 months ago

This could do with a documentation note at least? I don't think it's mentioned anywhere that pybind11.h should be first (or at least that it includes Python.h and has the same requirements as it does).

DWesl commented 1 month ago

doc/limitations.rst or doc/basics.rst look like decent places for such a warning. Do the maintainers have a preference on which place the warning goes?

pybind / pybind11