Fix Mingw64 build and add a CI job to test it #3132

jeromerobert · 2021-07-18T12:17:12Z

This is a tentative to fix mingw64 build and add a CI job to test it. So far it does not work. I would need some help or explanation to fix it.

A __declspec(dllexport) symbol cannot be hidden, it must have the default visibility (this is required by gcc). 3ac690b88 make builtin_exception __declspec(dllexport) while the pybind11 namespace is visibility(hidden).
I get a huge amount of deprecation like warning: 'pybind11::object::object(pybind11::handle, bool)' is deprecated: Use reinterpret_borrow<object>() or reinterpret_steal<object>()

This is the follow-up.

Suggested changelog entry:

* Fix Mingw64 and add to the CI testing matrix.

jeromerobert · 2021-07-18T12:36:04Z

A __declspec(dllexport) symbol cannot be hidden, it must have the default visibility (this is required by gcc). 3ac690b88 make builtin_exception __declspec(dllexport) while the pybind11 namespace is visibility(hidden).

An example from the GHA log:

D:/a/pybind11/pybind11/include/pybind11/detail/common.h:735:23: error: 'dllexport' implies default visibility, but 'class pybind11::builtin_exception' has already been declared with a different visibility
  735 | class PYBIND11_EXPORT builtin_exception : public std::runtime_error {
      |                       ^~~~~~~~~~~~~~~~~

Should I disable exception dllexport on Mingw64 or make the pybind11 namespace default visible ? Would @oraluben know ?

oraluben · 2021-07-19T01:00:03Z

Sorry I have few idea about the issue here, I only tested against gcc on Linux (with __attribute__), I guess the root cause could be GCC handling __declspec(dllexport) and __attribute__(visibility(hidden)) differently?

If that's the case then I think disable exception dllexport on Mingw64 (or any other gcc build on Windows) is a workaround good enough.

pybind11/setup_helpers.py

jeromerobert · 2021-07-29T09:06:18Z

May be CMAKE_INTERPROCEDURAL_OPTIMIZATION=OFF should moved to pybind11NewTools.cmake (i.e. LTO disabled by default for MINGW). What's your feeling ?

henryiii · 2021-07-29T12:44:50Z

I think we should handle it in pybind11Common, when we add the LTO flags. I"m trying just scaling it back a little first.

henryiii · 2021-07-29T15:02:54Z

Eventually, we might be able to come up with the correct LTO flags for MinGW, but for now, I think this is a clear improvement.

include/pybind11/detail/common.h

include/pybind11/pytypes.h

rwgk

I looked at the logs ... a ton of errors, wow.

include/pybind11/detail/common.h

See https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-python/0099-Change-the-get_platform-method-in-sysconfig-and-dist.patch

This is a fix for errors like: D:/a/pybind11/pybind11/include/pybind11/detail/common.h:735:23: error: 'dllexport' implies default visibility, but 'class pybind11::builtin_exception' has already been declared with a different visibility 735 | class PYBIND11_EXPORT builtin_exception : public std::runtime_error { | ^~~~~~~~~~~~~~~~~

jeromerobert · 2021-07-30T06:52:16Z

Oops it looks like a pushed PYBIND11_EXPORT_EXCEPTION without having read the last hours discussions.

jeromerobert · 2021-07-30T07:07:07Z

Fortunately this seems to be what you expect.

I looked at the logs ... a ton of errors, wow.

You mean tons of warnings ? Yes, I don't understand why (and where) object(handle h, bool is_borrowed) is called.

rwgk · 2021-07-30T10:53:13Z

Fortunately this seems to be what you expect.

I looked at the logs ... a ton of errors, wow.

You mean tons of warnings ?

On all (or at least most) other platforms we're using -Werror or equivalent.

Yes, I don't understand why (and where) object(handle h, bool is_borrowed) is called.

Do you have an interactive setup? A trick I often use is to add some extra arg(s) to the function, then build with --keep-going, it'll show all callers. Doesn't always work, but this looks like it could.

-object(handle h, bool is_borrowed)
+object(handle h, bool is_borrowed, const std::string &)

jeromerobert · 2021-07-30T12:57:14Z

I tried to apply the following patch which remove all deprecated member. All deprecation warnings disappeared and I get no error.

diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h
index 20d119f..b65ecd8 100644
--- a/include/pybind11/attr.h
+++ b/include/pybind11/attr.h
@@ -41,8 +41,6 @@ struct sibling { handle value; sibling(const handle &value) : value(value.ptr())
 /// Annotation indicating that a class derives from another given type
 template <typename T> struct base {

-    PYBIND11_DEPRECATED("base<T>() was deprecated in favor of specifying 'T' as a template argument to class_")
-    base() { } // NOLINT(modernize-use-equals-default): breaks MSVC 2015 when adding an attribute
 };

 /// Keep patient alive while nurse lives
@@ -61,10 +59,6 @@ struct buffer_protocol { };
 struct metaclass {
     handle value;

-    PYBIND11_DEPRECATED("py::metaclass() is no longer required. It's turned on by default now.")
-    // NOLINTNEXTLINE(modernize-use-equals-default): breaks MSVC 2015 when adding an attribute
-    metaclass() {}
-
     /// Override pybind11's default metaclass
     explicit metaclass(handle value) : value(value) { }
 };
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 139a411..a04f9aa 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -984,16 +984,6 @@ class module_ : public object {
 public:
     PYBIND11_OBJECT_DEFAULT(module_, object, PyModule_Check)

-    /// Create a new top-level Python module with the given name and docstring
-    PYBIND11_DEPRECATED("Use PYBIND11_MODULE or module_::create_extension_module instead")
-    explicit module_(const char *name, const char *doc = nullptr) {
-#if PY_MAJOR_VERSION >= 3
-        *this = create_extension_module(name, doc, new PyModuleDef());
-#else
-        *this = create_extension_module(name, doc, nullptr);
-#endif
-    }
-
     /** \rst
         Create Python binding for a new function within the module scope. ``Func``
         can be a plain C++ function, a function pointer, or a lambda function. For
diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h
index 4cf606e..3fa36e4 100644
--- a/include/pybind11/pytypes.h
+++ b/include/pybind11/pytypes.h
@@ -232,8 +232,6 @@ protected:
 class object : public handle {
 public:
     object() = default;
-    PYBIND11_DEPRECATED("Use reinterpret_borrow<object>() or reinterpret_steal<object>()")
-    object(handle h, bool is_borrowed) : handle(h) { if (is_borrowed) inc_ref(); }
     /// Copy constructor; always increases the reference count
     object(const object &o) : handle(o) { inc_ref(); }
     /// Move constructor; steals the object from ``other`` and preserves its reference count
@@ -854,8 +852,6 @@ PYBIND11_NAMESPACE_END(detail)
 //       isn't even used.
 #define PYBIND11_OBJECT_COMMON(Name, Parent, CheckFun) \
     public: \
-        PYBIND11_DEPRECATED("Use reinterpret_borrow<"#Name">() or reinterpret_steal<"#Name">()") \
-        Name(handle h, bool is_borrowed) : Parent(is_borrowed ? Parent(h, borrowed_t{}) : Parent(h, stolen_t{})) { } \
         Name(handle h, borrowed_t) : Parent(h, borrowed_t{}) { } \
         Name(handle h, stolen_t) : Parent(h, stolen_t{}) { } \
         PYBIND11_DEPRECATED("Use py::isinstance<py::python_type>(obj) instead") \
@@ -1292,8 +1288,6 @@ public:
 class capsule : public object {
 public:
     PYBIND11_OBJECT_DEFAULT(capsule, object, PyCapsule_CheckExact)
-    PYBIND11_DEPRECATED("Use reinterpret_borrow<capsule>() or reinterpret_steal<capsule>()")
-    capsule(PyObject *ptr, bool is_borrowed) : object(is_borrowed ? object(ptr, borrowed_t{}) : object(ptr, stolen_t{})) { }

     explicit capsule(const void *value, const char *name = nullptr, void (*destructor)(PyObject *) = nullptr)
         : object(PyCapsule_New(const_cast<void *>(value), name, destructor), stolen_t{}) {
@@ -1301,13 +1295,6 @@ public:
             pybind11_fail("Could not allocate capsule object!");
     }

-    PYBIND11_DEPRECATED("Please pass a destructor that takes a void pointer as input")
-    capsule(const void *value, void (*destruct)(PyObject *))
-        : object(PyCapsule_New(const_cast<void*>(value), nullptr, destruct), stolen_t{}) {
-        if (!m_ptr)
-            pybind11_fail("Could not allocate capsule object!");
-    }
-
     capsule(const void *value, void (*destructor)(void *)) {
         m_ptr = PyCapsule_New(const_cast<void *>(value), nullptr, [](PyObject *o) {
             auto destructor = reinterpret_cast<void (*)(void *)>(PyCapsule_GetContext(o));

jeromerobert · 2021-07-30T13:02:07Z

Here is the very first warning:

C:/msys2/home/jerome/pybind11/include/pybind11/pytypes.h: In constructor 'pybind11::object::object(pybind11::handle, bool)':
C:/msys2/home/jerome//pybind11/include/pybind11/pytypes.h:236:82: warning: 'pybind11::object::object(pybind11::handle, bool)' is deprecated: Use reinterpret_borrow<object>() or reinterpret_steal<object>() [-Wdeprecated
-declarations]
  236 |     object(handle h, bool is_borrowed) : handle(h) { if (is_borrowed) inc_ref(); }
      |                                                                                  ^
C:/msys2/home/jerome/pybind11/include/pybind11/pytypes.h:236:5: note: declared here
  236 |     object(handle h, bool is_borrowed) : handle(h) { if (is_borrowed) inc_ref(); }
      |     ^~~~~~

It looks that a warning is triggered each time a deprecated member is declared whether it is used or not. That does not make sens. Also, I cannot reproduce that on simple example.

rwgk · 2021-07-30T13:14:09Z

I'd be fine with #if !defined(__MINGW32__) around the deprecated code.

henryiii · 2021-07-30T13:16:22Z

It looks that a warning is triggered each time a deprecated member is declared whether it is used or not. That does not make sense.

I noticed this too, and don't know why it's behaving this way.

I'd be fine with #if !defined(MINGW32) around the deprecated code.

As long as you get the tests to pass, I'd be okay with that too. Though this seems like a weird bug, I wonder if there's a flag we are passing that's affecting this. @jeromerobert have you compared the compile commands we use with your simple example?

(Just leaving it for now is also okay, or doing it in a follow up)

It trigger many warnings for unknown reasons

jeromerobert · 2021-07-30T13:30:41Z

I'd be fine with #if !defined(MINGW32) around the deprecated code.

I disabled PYBIND11_DEPRECATED on mingw32

have you compared the compile commands we use with your simple example

Yes. I copied them from the verbose make log.

henryiii · 2021-07-30T13:34:57Z

I disabled PYBIND11_DEPRECATED on mingw32

Actually that's the simplest and (in hindsight) obvious solution. :)

henryiii

Poke me if I don't notice when this finishes, and I'll merge & backport to the v2.7 branch.

jeromerobert · 2021-07-30T17:00:14Z

All green !

* mingw64 platform string is like mingw_xxx not "mingw" See https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-python/0099-Change-the-get_platform-method-in-sysconfig-and-dist.patch * Mingw: Do not dllexport exceptions This is a fix for errors like: D:/a/pybind11/pybind11/include/pybind11/detail/common.h:735:23: error: 'dllexport' implies default visibility, but 'class pybind11::builtin_exception' has already been declared with a different visibility 735 | class PYBIND11_EXPORT builtin_exception : public std::runtime_error { | ^~~~~~~~~~~~~~~~~ * GHA: Test Mingw64 build * fix: avoid thin binaries on mingw * fix: drop lto on MinGW * Mingw64: disable PYBIND11_DEPRECATED It trigger many warnings for unknown reasons Co-authored-by: Henry Schreiner <[email protected]>

jeromerobert requested a review from henryiii as a code owner July 18, 2021 12:17

jeromerobert mentioned this pull request Jul 18, 2021

GHA: Test pip install with Mingw64 python and gcc pybind/python_example#75

Merged

jeromerobert marked this pull request as draft July 18, 2021 12:20

jeromerobert force-pushed the mingw64 branch from 370a724 to b97d5b1 Compare July 18, 2021 12:37

henryiii reviewed Jul 28, 2021

View reviewed changes

pybind11/setup_helpers.py Show resolved Hide resolved

jeromerobert force-pushed the mingw64 branch from a5c37b7 to 75b81a6 Compare July 29, 2021 08:50

jeromerobert marked this pull request as ready for review July 29, 2021 09:02

henryiii requested review from Skylion007 and rwgk July 29, 2021 15:03

rwgk reviewed Jul 29, 2021

View reviewed changes

include/pybind11/detail/common.h Outdated Show resolved Hide resolved

include/pybind11/pytypes.h Outdated Show resolved Hide resolved

rwgk reviewed Jul 30, 2021

View reviewed changes

include/pybind11/detail/common.h Outdated Show resolved Hide resolved

include/pybind11/detail/common.h Show resolved Hide resolved

jeromerobert and others added 5 commits July 30, 2021 08:46

mingw64 platform string is like mingw_xxx not "mingw"

7e2f90e

See https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-python/0099-Change-the-get_platform-method-in-sysconfig-and-dist.patch

GHA: Test Mingw64 build

d874307

fix: avoid thin binaries on mingw

f7f6018

fix: drop lto on MinGW

597119c

jeromerobert force-pushed the mingw64 branch from e0a2017 to 597119c Compare July 30, 2021 06:47

Mingw64: disable PYBIND11_DEPRECATED

12a4d2c

It trigger many warnings for unknown reasons

rwgk approved these changes Jul 30, 2021

View reviewed changes

henryiii approved these changes Jul 30, 2021

View reviewed changes

henryiii merged commit 9e8a741 into pybind:master Jul 30, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Jul 30, 2021

This was referenced Aug 19, 2021

pybind11 v2.7 mingw issue spatial-model-editor/spatial-model-editor#604

Closed

extend msys2 mingw CI #3207

Merged

henryiii removed the needs changelog Possibly needs a changelog entry label Sep 16, 2021

rwgk mentioned this pull request Oct 23, 2022

[BUG]: ABI incompatibility between 2.9.2 and 2.10 #4105

Closed

3 tasks

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Mingw64 build and add a CI job to test it #3132

Fix Mingw64 build and add a CI job to test it #3132

jeromerobert commented Jul 18, 2021 •

edited by henryiii

Loading

jeromerobert commented Jul 18, 2021

oraluben commented Jul 19, 2021

jeromerobert commented Jul 29, 2021

henryiii commented Jul 29, 2021

henryiii commented Jul 29, 2021

rwgk left a comment

jeromerobert commented Jul 30, 2021 •

edited

Loading

jeromerobert commented Jul 30, 2021

rwgk commented Jul 30, 2021

jeromerobert commented Jul 30, 2021

jeromerobert commented Jul 30, 2021

rwgk commented Jul 30, 2021

henryiii commented Jul 30, 2021 •

edited

Loading

jeromerobert commented Jul 30, 2021

henryiii commented Jul 30, 2021 •

edited

Loading

henryiii left a comment

jeromerobert commented Jul 30, 2021

Fix Mingw64 build and add a CI job to test it #3132

Fix Mingw64 build and add a CI job to test it #3132

Conversation

jeromerobert commented Jul 18, 2021 • edited by henryiii Loading

Suggested changelog entry:

jeromerobert commented Jul 18, 2021

oraluben commented Jul 19, 2021

jeromerobert commented Jul 29, 2021

henryiii commented Jul 29, 2021

henryiii commented Jul 29, 2021

rwgk left a comment

Choose a reason for hiding this comment

jeromerobert commented Jul 30, 2021 • edited Loading

jeromerobert commented Jul 30, 2021

rwgk commented Jul 30, 2021

jeromerobert commented Jul 30, 2021

jeromerobert commented Jul 30, 2021

rwgk commented Jul 30, 2021

henryiii commented Jul 30, 2021 • edited Loading

jeromerobert commented Jul 30, 2021

henryiii commented Jul 30, 2021 • edited Loading

henryiii left a comment

Choose a reason for hiding this comment

jeromerobert commented Jul 30, 2021

jeromerobert commented Jul 18, 2021 •

edited by henryiii

Loading

jeromerobert commented Jul 30, 2021 •

edited

Loading

henryiii commented Jul 30, 2021 •

edited

Loading

henryiii commented Jul 30, 2021 •

edited

Loading