Skip to content

[Bug]: Cygwin build fails due to use of Windows-only functions in _tkagg.cpp #22997

New issue
New issue
Closed
#23066
Closed
[Bug]: Cygwin build fails due to use of Windows-only functions in _tkagg.cpp#22997
#23066
Labels
OS: Microsoft🌱 Good first issueOpen a pull request against these issues if there are no active ones!
Milestone
v3.5.3

Description

@DWesl
DWesl
opened on May 6, 2022

Bug summary

When the build gets to https://github.com/matplotlib/matplotlib/blob/main/src/_tkagg.cpp#L262-L273 on Cygwin, the build fails with a few goto crosses initialization warnings, which are easy to fix (closed by #23051), and two error: ‘PyErr_SetFromWindowsErr’ was not declared in this scope, which are less easy to fix.

Code for reproduction

pip install matplotlib

Actual outcome

...
lots of compiler command lines and some warnings (`array subscript has type 'char'`)
...
          gcc ...  -I/usr/include/python3.8 -c src/_tkagg.cpp -o build/temp.cygwin-3.3.4-x86_64-3.8/matplotlib.backends._tkagg/src/_tkagg.o -fvisibility=hidden -flto
          src/_tkagg.cpp: In function ‘void load_tkinter_funcs()’:
          src/_tkagg.cpp:263:9: error: ‘PyErr_SetFromWindowsErr’ was not declared in this scope
            263 |         PyErr_SetFromWindowsErr(0);
                |         ^~~~~~~~~~~~~~~~~~~~~~~
          src/_tkagg.cpp:271:9: error: ‘PyErr_SetFromWindowsErr’ was not declared in this scope
            271 |         PyErr_SetFromWindowsErr(0);
                |         ^~~~~~~~~~~~~~~~~~~~~~~
          src/_tkagg.cpp:283:1: error: jump to label ‘exit’
            283 | exit:
                | ^~~~
          src/_tkagg.cpp:272:14: note:   from here
            272 |         goto exit;
                |              ^~~~
          src/_tkagg.cpp:274:26: note:   crosses initialization of ‘bool tk_ok’
            274 |     bool tcl_ok = false, tk_ok = false;
                |                          ^~~~~
          src/_tkagg.cpp:274:10: note:   crosses initialization of ‘bool tcl_ok’
            274 |     bool tcl_ok = false, tk_ok = false;
                |          ^~~~~~
          src/_tkagg.cpp:283:1: error: jump to label ‘exit’
            283 | exit:
                | ^~~~
          src/_tkagg.cpp:268:14: note:   from here
            268 |         goto exit;
                |              ^~~~
          src/_tkagg.cpp:274:26: note:   crosses initialization of ‘bool tk_ok’
            274 |     bool tcl_ok = false, tk_ok = false;
                |                          ^~~~~
          src/_tkagg.cpp:274:10: note:   crosses initialization of ‘bool tcl_ok’
            274 |     bool tcl_ok = false, tk_ok = false;
                |          ^~~~~~
          src/_tkagg.cpp:283:1: error: jump to label ‘exit’
            283 | exit:
                | ^~~~
          src/_tkagg.cpp:264:14: note:   from here
            264 |         goto exit;
                |              ^~~~
          src/_tkagg.cpp:274:26: note:   crosses initialization of ‘bool tk_ok’
            274 |     bool tcl_ok = false, tk_ok = false;
                |                          ^~~~~
          src/_tkagg.cpp:274:10: note:   crosses initialization of ‘bool tcl_ok’
            274 |     bool tcl_ok = false, tk_ok = false;
                |          ^~~~~~
          error: command '/usr/bin/gcc' failed with exit code 1
...
pip reminds me this is not pip's fault

Expected outcome

...
Successfully installed collected packages: matplotlib

Additional information

Cygwin 3.3.4
GCC 11.2.0
Python 3.8.12

I've installed matplotlib in earlier versions. #17415 rather suggests that Cygwin builds worked at that time, and probably made using the default backend somewhat easier. #22445 seems to be where the PyErr_SetFromWindowsErr calls came in.

I've attempted to create a dummy PyErr_SetFromWindowsErr on Cygwin, but, given the number of failures in the test suite, I probably didn't do it right.

Operating system

Cygwin 3.3.4 on Windows 10

Matplotlib Version

3.5.2

Matplotlib Backend

TkAgg

Python version

3.8.12

Jupyter version

N/A

Installation

pip

Activity

  1. oscargus
    added
    OS: Microsoft
    on May 6, 2022

  2. tacaswell commented on May 6, 2022

    @tacaswell
    tacaswell
    on May 6, 2022
    Member

    This was last touched in 9d325ae

    I suspect we "just" need some better gating around cygwin?

  3. tacaswell
    added
    🌱 Good first issueOpen a pull request against these issues if there are no active ones!
    on May 6, 2022
  4. tacaswell
    added this to the v3.5.3 milestone on May 6, 2022

  5. tacaswell commented on May 6, 2022

    @tacaswell
    tacaswell
    on May 6, 2022
    Member

    Actions here:

    • #ifndef protect the windows only functions and replace them with PyErr_SetString (or similar)
    • sort out how to get at least 1 cygwin build running on our CI.

  6. DWesl commented on May 6, 2022

    @DWesl
    DWesl
    on May 6, 2022
    ContributorAuthor

    That looks like a good summary. Where should the Cygwin CI run end up? .github/workflows/tests.yml? nightlies.yml or a new file are the other likely spots.

  7. tacaswell commented on May 6, 2022

    @tacaswell
    tacaswell
    on May 6, 2022
    Member

    If it can be done on GHA, add it to .github/workflows/tests.yml (we also have appveyor and azure setup if those are easier to work with). I would very much prefer to not pick up yet another CI service (YACS .... that would be a good name of a new CI service ;) ).

    I assume you found this already, but we have

    matplotlib/src/_tkagg.cpp

    Lines 15 to 27 in 7bb3ff5

    #ifdef _WIN32
    #define WIN32_DLL
    #endif
    #ifdef __CYGWIN__
    /*
    * Unfortunately cygwin's libdl inherits restrictions from the underlying
    * Windows OS, at least currently. Therefore, a symbol may be loaded from a
    * module by dlsym() only if it is really located in the given modile,
    * dependencies are not included. So we have to use native WinAPI on Cygwin
    * also.
    */
    #define WIN32_DLL
    #endif
    at the top so we intentionally conflating the two.

  8. DWesl commented on May 6, 2022

    @DWesl
    DWesl
    on May 6, 2022
    ContributorAuthor

    I can do CI on GHA. I'm less sure about the fixing-my-problem part.

    Yes. That's from #17415; basically Cygwin inherits Windows's dynamic library semantics, so treating them the same (for related issues) generally solves more problems than it causes.

  9. anntzer commented on May 7, 2022

    @anntzer
    anntzer
    on May 7, 2022
    Contributor

    I guess you need something like

    diff --git i/src/_tkagg.cpp w/src/_tkagg.cpp
index 5a5616bea5..c54463dca7 100644
--- i/src/_tkagg.cpp
+++ w/src/_tkagg.cpp
@@ -24,6 +24,8 @@
  * also.
  */
 #define WIN32_DLL
+#define PyErr_SetFromWindowsErr(ierr) \
+    PyErr_SetString(PyExc_OSError, "Call to EnumProcessModules failed")
 #endif
 
 #ifdef WIN32_DLL

    (not the cleanest solution, but inlining all the implementation of PyErr_SetFromWindowsErr from cpython's errors.c seems a bit overkill; I would also suggest opening an issue on the cpython tracker to make PyErr_SetFromWindowsErr (and related functions) available on cygwin too -- from a first look (I may well be mistaken), it's just an oversight on their side?)

  10. DWesl commented on May 10, 2022

    @DWesl
    DWesl
    on May 10, 2022
    ContributorAuthor

    Would a function like:

    static inline PyObject *PyErr_SetFromWindowsErr(int ierr) {
    return PyErr_SetString(PyExc_OSError, "Call to EnumProcessModules failed");
}

    also work? Compiler error reporting around preprocessor tokens has gotten better recently, but reporting around functions is often better still.

    On a related note, should I make a separate PR for these fixes, since trying to make get a Cygwin CI working in #22999 is still fighting me?

  11. anntzer commented on May 18, 2022

    @anntzer
    anntzer
    on May 18, 2022
    Contributor

    I guess so? (x2)

  12. DWesl commented on May 18, 2022

    @DWesl
    DWesl
    on May 18, 2022
    ContributorAuthor

    The barebones function based on the macro proposed earlier has been proposed as PR #23066.

  13. tacaswell commented on May 18, 2022

    @tacaswell
    tacaswell
    on May 18, 2022
    Member

    Sorry my request for CI sent you down a rabbit hole! Thank you for your work and patience
    .

  14. DWesl commented on May 18, 2022

    @DWesl
    DWesl
    on May 18, 2022
    ContributorAuthor

    I've done a few Cygwin CI setups, so I thought that would be simpler. I wasn't expecting that updating the Libtool configuration of FreeType would fail.

  15. anntzer
    closed this as completedin #23066on Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    OS: Microsoft🌱 Good first issueOpen a pull request against these issues if there are no active ones!

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions