Fix build with LTO disabled in environment

[QuLogic]

PR Summary

Fixes #19227.

PR Checklist

• [n/a] Has pytest style unit tests (and pytest passes).
• Is Flake 8 compliant (run flake8 on changed files to check).
• [n/a] New features are documented, with examples if plot related.
• [n/a] Documentation is sphinx and numpydoc compliant (the docs should build without error).
• Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
• [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
• [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

[jklymak]

Do we have a setup to test this on?

[QuLogic]

It seems a bit weird to drop into a single test invocation, but we could maybe do so. Basically running CFLAGS=-fno-lto pip install -e .

[tacaswell]

we could run just of the existing builds with that flag? The issues reports that it builds clean but then fails to import so we need to run something.

That would also help us catch any (hopefully very edge) cases where LTO changes behavior on us?

[QuLogic]

So I tested by just adding CFLAGS everywhere and strangely, it only fails on the subprocess tests. I can reproduce locally, so will figure that out tomorrow.

[QuLogic]

Ah, I think I've figured it out now; Python compiles the files with gcc (using CFLAGS) and then compiles the final library with g++ (using CXXFLAGS). This is a bit weird to me since they have .cpp extensions, but I guess it works. However, that means that the second call does not get a -fno-lto flag (and neither does it see our lto flags since this PR would disable them) and by default it at least does some LTO/visibility stuff.

So I'm going to say that this is not our bug to fix. If you want to set *FLAGS, you need to set all of them consistently.

I will setup the test to do the same.

[QuLogic]

Nope, sorry, I got that all wrong. The problem is that tkagg.cpp includes py_converters.h due to #12569, and #19094 removes the NumPy array reference to silence a warning, but means that the symbol does not exist for the stuff in py_converters.h.

With LTO, the NumPy-referencing converters are stripped due to being unused, and apparently, with no LTO option specified, that also happens. But with -fno-lto, no unused converters are stripped and this results in a missing symbol.

[QuLogic]

This now fixes the build with LTO disabled in the environment, by splitting the converters into two separate files so that the tkagg backend does not need to use NumPy. It also adds the CFLAGS in one test environment.

[anntzer]

(I don't mind reverting #19094 if that makes things simpler -- it's only a compile-time warning anyways...)

[story645]

merge on passing? scratch, I'm not sure if this PR is what's causing the fail since it's failing on an install of numpy

not sure if PR is what's causing fail

[QuLogic]

Pre-release failures are tracked in #19352.

[story645]

Seems to be the other windows build that's failing?

[jklymak]

@anntzer can you review and merge? I don't quite understand the file split, nor do I really track the compile time flag subtleties. Whereas this change seems to have been necessitated by a change you recently made.

[anntzer]

I honestly don't really follow what's happening, and as stated above, I'd rather just revert #19094 if that's sufficient, as #19094 really just suppressed a harmless build-time warning.

Or the following should work too? (possibly still with the setup.py additional checks)

diff --git a/src/_tkagg.cpp b/src/_tkagg.cpp
index 3e5793f03..60dc53d6e 100644
--- a/src/_tkagg.cpp
+++ b/src/_tkagg.cpp
@@ -33,14 +33,10 @@
 #define dlsym GetProcAddress
 #else
 #include <dlfcn.h>
-// Suppress -Wunused-function on POSIX, but not on Windows where that would
-// lead to PY_ARRAY_UNIQUE_SYMBOL being an unresolved external.
-#define NO_IMPORT_ARRAY
 #endif
 
 // Include our own excerpts from the Tcl / Tk headers
 #include "_tkmini.h"
-#include "py_converters.h"
 
 // Global vars for Tk functions.  We load these symbols from the tkinter
 // extension module or loaded Tk libraries at run-time.
@@ -49,19 +45,23 @@ static Tk_PhotoPutBlock_NoComposite_t TK_PHOTO_PUT_BLOCK_NO_COMPOSITE;
 
 static PyObject *mpl_tk_blit(PyObject *self, PyObject *args)
 {
+    PyObject *py_interp;
     Tcl_Interp *interp;
     char const *photo_name;
     int height, width;
+    PyObject *py_data_ptr;
     unsigned char *data_ptr;
     int o0, o1, o2, o3;
     int x1, x2, y1, y2;
     Tk_PhotoHandle photo;
     Tk_PhotoImageBlock block;
-    if (!PyArg_ParseTuple(args, "O&s(iiO&)(iiii)(iiii):blit",
-                          convert_voidptr, &interp, &photo_name,
-                          &height, &width, convert_voidptr, &data_ptr,
+    if (!PyArg_ParseTuple(args, "Os(iiO)(iiii)(iiii):blit",
+                          &py_interp, &photo_name,
+                          &height, &width, &py_data_ptr,
                           &o0, &o1, &o2, &o3,
-                          &x1, &x2, &y1, &y2)) {
+                          &x1, &x2, &y1, &y2)
+        || !(interp = (Tcl_Interp *)PyLong_AsVoidPtr(py_interp))
+        || !(data_ptr = (unsigned char *)PyLong_AsVoidPtr(py_data_ptr))) {
         goto exit;
     }
     if (!(photo = TK_FIND_PHOTO(interp, photo_name))) {
diff --git a/src/py_converters.cpp b/src/py_converters.cpp
index ace8332dd..a4c0b1909 100644
--- a/src/py_converters.cpp
+++ b/src/py_converters.cpp
@@ -94,13 +94,6 @@ int convert_from_attr(PyObject *obj, const char *name, converter func, void *p)
     return 1;
 }
 
-int convert_voidptr(PyObject *obj, void *p)
-{
-    void **val = (void **)p;
-    *val = PyLong_AsVoidPtr(obj);
-    return *val != NULL ? 1 : !PyErr_Occurred();
-}
-
 int convert_double(PyObject *obj, void *p)
 {
     double *val = (double *)p;
diff --git a/src/py_converters.h b/src/py_converters.h
index 33af43a47..2c19acdaa 100644
--- a/src/py_converters.h
+++ b/src/py_converters.h
@@ -22,7 +22,6 @@ typedef int (*converter)(PyObject *, void *);
 int convert_from_attr(PyObject *obj, const char *name, converter func, void *p);
 int convert_from_method(PyObject *obj, const char *name, converter func, void *p);
 
-int convert_voidptr(PyObject *obj, void *p);
 int convert_double(PyObject *obj, void *p);
 int convert_bool(PyObject *obj, void *p);
 int convert_cap(PyObject *capobj, void *capp);

[QuLogic]

Both #19094 and reverting it are just workarounds for the real problem, which is that _tkagg.so is compiling in NumPy functions and thus should be linked with NumPy, when it doesn't need it. That why this splits converters between simple Python ones and ones that really need NumPy.

If you prefer to inline convert_voidptr, that does work too; it's only used in tkagg.

[anntzer]

If you prefer to inline convert_voidptr, that does work too; it's only used in tkagg.

Let's go for the simpler(?) solution then?

[anntzer]

postci

[QuLogic]

CI failures are known issues.