MAINT: update meson.build to make it work on IBM i system #17193
Conversation
ilayn
changed the title
ilayn
added
the
Meson
There was a problem hiding this comment.
Hi @zheddie, thanks for the PR. I asked for two changes (threads dependency and using add_project_argument), with that it should be fine to merge. I do recommend trying to upstream this into Meson, so we can remove it again here. That way, it will also help Pandas, NumPy and everyone else who is moving to Meson.
meson.build
Outdated
| @@ -50,6 +50,13 @@ m_dep = cc.find_library('m', required : false) | |||
| if m_dep.found() | |||
| add_project_link_arguments('-lm', language : 'c') | |||
| endif | |||
| if host_machine.system() == 'os400' | |||
| add_global_arguments('-pthread', language : 'cpp') | |||
| add_global_arguments('-D__STDC_FORMAT_MACROS', language : 'cpp') | |||
There was a problem hiding this comment.
These two lines should use add_project_argument rather than add_global_argument.
There was a problem hiding this comment.
Adding __STDC_FORMAT_MACROS is fine - I guess the OS/400 C++ compiler is a bit outdated (modern C++ compilers should unconditionally define what's behind this macro - see https://stackoverflow.com/questions/8132399/how-to-printf-uint64-t-fails-with-spurious-trailing-in-format/8132440#8132440).
There was a problem hiding this comment.
These two lines should use
add_project_argumentrather thanadd_global_argument.
Yeah. I would try add_project_argument. BTW, in fact, by checking the build.ninja , we can find that "-pthread" could be added for some c++ source code, while, it only failed on some files , such as control.cc. To be honest , I didn't figure out why mason treat control.cc specially.
Adding
__STDC_FORMAT_MACROSis fine - I guess the OS/400 C++ compiler is a bit outdated (modern C++ compilers should unconditionally define what's behind this macro - see https://stackoverflow.com/questions/8132399/how-to-printf-uint64-t-fails-with-spurious-trailing-in-format/8132440#8132440).
Yes, this is exactly.
There was a problem hiding this comment.
These two lines should use
add_project_argumentrather thanadd_global_argument.
Last change: can you push this update?
| @@ -50,6 +50,13 @@ m_dep = cc.find_library('m', required : false) | |||
| if m_dep.found() | |||
| add_project_link_arguments('-lm', language : 'c') | |||
| endif | |||
| if host_machine.system() == 'os400' | |||
There was a problem hiding this comment.
I'll note that this string isn't documented in https://mesonbuild.com/Reference-tables.html#operating-system-names, so not guaranteed to remain stable (although in practice, it shouldn't change).
There was a problem hiding this comment.
yeah. We didn't focus on mason , as we can smoothly(almost, except for the unkown CPU type.
All could using `#include <thread>` needs to have this `thread_dep` dependency. HiGHS code was missing it, and for `stats/_qmc_cy.pyx` it was incorrectly specified. It's wrong in the distutils build as well, but let's ignore that since it's much harder to change and we'll get rid of that build soon. See scipygh-17193 for the type of compile error this causes.
All could using `#include <thread>` needs to have this `thread_dep` dependency. HiGHS code was missing it, and for `stats/_qmc_cy.pyx` it was incorrectly specified. It's wrong in the distutils build as well, but let's ignore that since it's much harder to change and we'll get rid of that build soon. See gh-17193 for the type of compile error this causes. [ci skip]
rgommers
added
backport-candidate
| add_global_arguments('-D__STDC_FORMAT_MACROS', language : 'cpp') | ||
| add_project_link_arguments('-Wl,-bnotextro', language : 'c') | ||
| add_project_link_arguments('-Wl,-bnotextro', language : 'cpp') | ||
| add_project_link_arguments('-Wl,-bnotextro', language : 'fortran') |
There was a problem hiding this comment.
It was a little tricky to find what this flag means. From the AIX 7.2 docs for ld:
notextro or nro: Does not check to ensure that there are no load time relocation entries for the text section of the output object file. This is the default.
So I guess on OS/400 it is not the default, unlike on AIX, and somehow this is a problem? For completeness it'd be nice to see the build error that shows up in the absence of this flag @zheddie.
There was a problem hiding this comment.
Yes. here's the error message if without those options:
[19/1566] Compiling C object scipy/special/libcephes.a.p/cephes_const.c.o
[20/1566] Linking target scipy/_lib/_test_ccallback.cpython-39.so
FAILED: scipy/_lib/_test_ccallback.cpython-39.so
cc -o scipy/_lib/_test_ccallback.cpython-39.so scipy/_lib/_test_ccallback.cpython-39.so.p/src__test_ccallback.c.o -Wl,-berok -Wl,-bnoipath -Wl,-bbigtoc -shared -fPIC -lm -Wl,-blibpath:/qopensys/pkgs/lib/gcc/powerpc-ibm-aix6.1.0.0/10:/qopensys/pkgs/lib/gcc:/qopensys/pkgs/lib:/lib:/usr/lib
ld: 0711-302 ERROR: Object scipy/_lib/_test_ccallback.cpython-39.so.p/src__test_ccallback.c.o, csect <.text>
The csect is part of the .text section, and relocation entries
from the csect have been written to the .loader section.
ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
collect2: error: ld returned 8 exit status
There was a problem hiding this comment.
Thanks. Interesting that it's AIX 6.1 with GCC, which seems to be working on other IBM hardware (I think, or we would have had more complaints by now) but not on IBM i system.
Anyway, this change looks good then. It's failing on the very first shared library being built, so it's going to fail on all of them. Hence adding a project-wide link argument seems fine.
There was a problem hiding this comment.
oh. Sorry, just noticed that I am still using the add_global_arguments(). It's a mis-match from my local system and build system. My test were donewith add_project_arguments() . So, reset the code change with correct one. Sorry about it.
As for the IBM i , yes, we are not AIX, and this is our first type trying using the pip3 directly on our system. Before that we are trying using the RPM to ship Scipy, which is not easy for us to keep pace with open source world.
zheddie
force-pushed
the
mesonsupporti
branch
from
95ad344
to
39e5559
Compare
rgommers
changed the title
All could using `#include <thread>` needs to have this `thread_dep` dependency. HiGHS code was missing it, and for `stats/_qmc_cy.pyx` it was incorrectly specified. It's wrong in the distutils build as well, but let's ignore that since it's much harder to change and we'll get rid of that build soon. See scipygh-17193 for the type of compile error this causes. [ci skip]
[ci skip] Co-authored-by: GavinZhang <zhanggan@cn.ibm.com> Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
[ci skip] Co-authored-by: GavinZhang <zhanggan@cn.ibm.com> Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
All could using `#include <thread>` needs to have this `thread_dep` dependency. HiGHS code was missing it, and for `stats/_qmc_cy.pyx` it was incorrectly specified. It's wrong in the distutils build as well, but let's ignore that since it's much harder to change and we'll get rid of that build soon. See scipygh-17193 for the type of compile error this causes. [ci skip]
tylerjereddy
removed
the
backport-candidate
Add extra options on IBM i to make the meson build/pip3 works. The changes are covered by host_machine.system() == 'os400', so, it would not affect any existing systems. Please help to review. Thanks.
@rgommers