8000 BLD: numpy-1.23.0rc1 wheels cannot be used to build scipy · Issue #21629 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BLD: numpy-1.23.0rc1 wheels cannot be used to build scipy #21629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
mattip opened this issue May 29, 2022 · 16 comments · Fixed by #21630
Closed

BLD: numpy-1.23.0rc1 wheels cannot be used to build scipy #21629

mattip opened this issue May 29, 2022 · 16 comments · Fixed by #21630

Comments

@mattip
Copy link
Member
mattip commented May 29, 2022

Continuation of #21360, related to #20880

The cibuildwheel-built wheels have a subtle problem with npymath.lib. The compilation adds a msvc-specific .drectve section to the library which mingw64 does not understand. This causes the build on scipy to fail. See scipy/scipy#16305. It seems they have a directive to link to py3.x.lib, which comes from pragma comment(lib,"python3XX.lib") in pyconfig.h. Now what changed in npymath that is including python headers?
@mattip
Copy link
Member Author
mattip commented May 29, 2022

I wonder if thedifference might be moving to a c++ compiler, which makes the static npymath.lib subtly different. xref #21367

@charris
Copy link
Member
charris commented May 29, 2022

Hmm, that's a thought. There are probably some nightly wheels from before that PR was merged.

@mattip
Co 8000 py link
Member Author
mattip commented May 29, 2022

It seems we need to back out #21367. I can build withthe wheel from numpy==1.23.0.dev0+1087.g0eaa40db3, which was built from the weekly build on Thurs April 28, but the build with the wheel from numpy==1.23.0.dev0+1125.g4c3958b98 from May 5 fails.

@seberg
Copy link
Member
seberg commented May 29, 2022

@serge-sans-paille ping in case you have an idea. I wonder if there might be any easy fix, such as making sure that the C++ side of it doesn't expose any symbols?

@mattip
Copy link
Member Author
mattip commented May 29, 2022

The very use of the MSVC c++ compiler I think injects new link directives into the npymath.lib headers, that mingw64 doesn't know how to deal with. There is no "link" because this is a static library, so the compiler directives are the union of the directives from each of the component *.obj files in the archive. These are added, I think by the change in the compiler.

   /FAILIFMISMATCH:_CRT_STDIO_ISO_WIDE_SPECIFIERS=0
   /FAILIFMISMATCH:_MSC_VER=1900
   /FAILIFMISMATCH:_ITERATOR_DEBUG_LEVEL=0
   /FAILIFMISMATCH:RuntimeLibrary=MD_DynamicRelease
   /DEFAULTLIB:msvcprt

I think we should not touch npymath.lib until we make it a dynamic library. xref #20880

Using dumpbin /DIRECTIVES npymath.lib on the file from 1.22, (that works) I get

>dumpbin /directives ..\numpy1.22.2\numpy\core\lib\npymath.lib
Microsoft (R) COFF/PE Dumper Version 14.29.30141.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file ..\numpy1.22.2\numpy\core\lib\npymath.lib

File Type: LIBRARY

   Linker Directives
   -----------------
   /DEFAULTLIB:python310.lib
   /DEFAULTLIB:MSVCRT
   /DEFAULTLIB:OLDNAMES

   Linker Directives
   -----------------
   /DEFAULTLIB:python310.lib
   /DEFAULTLIB:MSVCRT
   /DEFAULTLIB:OLDNAMES

   Linker Directives
   -----------------
   /DEFAULTLIB:python310.lib
   /DEFAULTLIB:MSVCRT
   /DEFAULTLIB:OLDNAMES

   Linker Directives
   -----------------
   /DEFAULTLIB:python310.lib
   /DEFAULTLIB:MSVCRT
   /DEFAULTLIB:OLDNAMES

  Summary

          18 .bss
         3B8 .chks64
          10 .data
         2B8 .debug$S
         12C .drectve
         630 .pdata
         2F0 .rdata
        8A1D .text$mn
         8F4 .xdata

and on the new one (that doesn't work) I get

>dumpbin /directives core\lib\npymath.lib
Microsoft (R) COFF/PE Dumper Version 14.29.30141.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file core\lib\npymath.lib

File Type: LIBRARY

   Linker Directives
   -----------------
   /FAILIFMISMATCH:_CRT_STDIO_ISO_WIDE_SPECIFIERS=0
   /DEFAULTLIB:python310.lib
   /FAILIFMISMATCH:_MSC_VER=1900
   /FAILIFMISMATCH:_ITERATOR_DEBUG_LEVEL=0
   /FAILIFMISMATCH:RuntimeLibrary=MD_DynamicRelease
   /DEFAULTLIB:msvcprt
   /DEFAULTLIB:MSVCRT
   /DEFAULTLIB:OLDNAMES

   Linker Directives
   -----------------
   /DEFAULTLIB:python310.lib
   /DEFAULTLIB:MSVCRT
   /DEFAULTLIB:OLDNAMES

   Linker Directives
   -----------------
   /DEFAULTLIB:python310.lib
   /DEFAULTLIB:MSVCRT
   /DEFAULTLIB:OLDNAMES

   Linker Directives
   -----------------
   /DEFAULTLIB:python310.lib
   /DEFAULTLIB:MSVCRT
   /DEFAULTLIB:OLDNAMES

  Summary

        15D8 .chks64
          18 .data
         2A0 .debug$S
         1F2 .drectve
         660 .pdata
         358 .rdata
        78A1 .text$mn
         8D4 .xdata

@charris
Copy link
Member
charris commented May 29, 2022

We can back it out, then build wheels manually for the 1.23.x branch, which should put the wheels in nightly where we can test them before the next rc.

@mattip mattip mentioned this issue May 29, 2022
Merged
@serge-sans-paille
Copy link
Contributor

@serge-sans-paille ping in case you have an idea. I wonder if there might be any easy fix, such as making sure that the C++ side of it doesn't expose any symbols?

I don't see libm in https://github.com/scipy/scipy/runs/6643118574?check_suite_focus=true#step:10:516 , that's where feraiseexcept should be defined, right?

@mattip
Copy link
Member Author
mattip commented May 29, 2022

The error there is coming from mingw64 linking with npymath.lib, which is built by msvc. mpymath.lib has .drectve sections that confuse mingw64.

@serge-sans-paille
Copy link
Contributor

ok, never met that... looks like incompatible toolchain combination of sort?

@serge-sans-paille
Copy link
Contributor

@mattip the only linker-specifc option we have forC++ is in core/steup.py

 # Forcing C language even though we have C++ sources.
 # It forces the C linker and don't link C++ runtime.
 language = 'c',

Maybe we could remove that and see what happens?

@mattip
Copy link
Member Author
mattip commented May 30, 2022

PR #21634 reverted the refactoring of c code to c++, which solved the problem.

@serge-sans-paille
Copy link
Contributor

@mattip is that revert only for the release? I guess it would be great to keep the refactoring and find the actual origin of the bug (?)

@seberg
Copy link
Member
seberg commented Jun 16, 2022

Right now it is also reverted on main, although the C++ file is still around, so you can swap it in by changing which line is commented out in setup.py.
I guess we could give it another shot, although it would be nice to have an idea for fixing?

@mattip
Copy link
Member Author
mattip commented Jun 16, 2022

I think we should be very careful about changing npymath until we make npymath a shared object. Also see #20880

@seberg
Copy link
Member
seberg commented Jun 16, 2022

What I am uncertain about is wehther npymath being linked statically is in some form important e.g. for in-lining of functions?

But assuming that there is a reason why a shared library is tricky, would the alternative be shipping npymath.a as a distinct package (that we just use a submodule probably)?

@mattip
Copy link
Member Author
mattip commented Jun 17, 2022

would the alternative be shipping npymath.a as a distinct package (that we just use a submodule probably)?

Probably best to continue the discussion about the path forward on issue #20880 so it gets more eyes.

@andyfaff andyfaff mentioned this issue Aug 5, 2022
Closed
@WarrenWeckesser WarrenWeckesser mentioned this issue Sep 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

MAINT: back out conversion of npymath component to c++ mattip/numpy
4 participants
@seberg @charris @mattip @serge-sans-paille
0