Skip to content
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.

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

Issues with python.extension_module() on Windows with MSVC #10775

Closed
dnicolodi opened this issue Sep 4, 2022 · 12 comments · Fixed by #10800
Closed

Issues with python.extension_module() on Windows with MSVC #10775

dnicolodi opened this issue Sep 4, 2022 · 12 comments · Fixed by #10800

Comments

@dnicolodi
Copy link
Member

I would like to build a simple Python extension. My meson.build looks something like this:

py_mod = import('python')
py = py_mod.find_installation()
py_dep = py.dependency()

py.extension_module(
  'siphash24', 'siphash24.pyx', 'c-siphash.c',
  install: true,
  dependencies: py_dep,
)

the full code can be found here https://github.com/dnicolodi/python-siphash24

Everything works well on Linux and macOS however something is off on Windows when compiling with MSVC:

[4/4] Linking target siphash24.cp310-win_amd64.pyd
   Creating library siphash24.cp310-win_amd64.lib and object siphash24.cp310-win_amd64.exp
[...]
Installing siphash24.cp310-win_amd64.pyd to C:\Users\RUNNER~1\AppData\Local\Temp\build-via-sdist-g78k95j9\siphash24-1.0.0\.mesonpy-b3ixowso\install\hostedtoolcache\windows\Python\3.10.6\x64\Lib\site-packages
Installing siphash24.cp310-win_amd64.lib to C:\Users\RUNNER~1\AppData\Local\Temp\build-via-sdist-g78k95j9\siphash24-1.0.0\.mesonpy-b3ixowso\install\hostedtoolcache\windows\Python\3.10.6\x64\Lib\site-packages
File 'siphash24.cp310-win_amd64.pdb' not found, skipping

It seems that (what I think are) debug symbols are generated with the wrong filename extension.

This is on the Github Actions runners with the default MSVC installation. I'm not familiar with development on Windows thus I really don't know what may be going wrong.

@rgommers
Copy link
Contributor

rgommers commented Sep 4, 2022

That is odd indeed. The build is configured with -Ddebug=false, so it shouldn't be looking for a .pdb file I'd think.

Perhaps a duplicate of gh-10639.

@rgommers
Copy link
Contributor

rgommers commented Sep 4, 2022

@dnicolodi if you build with meson setup build -Ddebug=false and then ninja -C build, does the error go away?

Meson uses intro-installed.json while meson-python uses intro-install-plan.json, and that discrepancy is perhaps the cause here.

I had a look at CI coverage, and there are extensive MSVC tests, but Cython is skipped there. Nevertheless, I'd expect that to work smoothly, because all it needs is compiling C code. I'll see if I can enable those tests.

@eli-schwartz
Copy link
Member

Cython is skipped there because the cython suite is only run if cython is installed, and it doesn't seem that cython is installed as part of e.g. ci/run.ps1.

But cython shouldn't be relevant here -- instead, consider test cases/python/2 extmodule/, which installs a pure C extension. It doesn't create a pdb file either, although this may be because it sets buildtype=release. We know it isn't building a pdb file in CI, because the installed list of files includes an implib but not a pdb, and the test harness would fail if it detects an additional file being installed that isn't in the test validation data.

@rgommers
Copy link
Contributor

rgommers commented Sep 4, 2022

Yes, fairly sure the .pdb file is due to the intro-install-plan issue link above - so the bug will only show up when building via meson-python. Nevertheless it seems nice to run the Cython tests on Windows. Giving that a go in gh-10776.

Side note: studying the Meson CI setup was quite enlightening, I think I'll try to borrow the way it installs the Intel Fortran compiler on Windows. That will fix a big gap in SciPy's CI coverage.

@eli-schwartz
Copy link
Member

Nevertheless it seems nice to run the Cython tests on Windows.

Yep, of course. :D

Side note: studying the Meson CI setup was quite enlightening, I think I'll try to borrow the way it installs the Intel Fortran compiler on Windows. That will fix a big gap in SciPy's CI coverage.

I myself was greatly enlightened by MODFLOW-USGS/modflow6#978 (comment), direct all credit to @Hofer-Julian and co.

@Hofer-Julian
Copy link
Contributor

And we took it from https://github.com/oneapi-src/oneapi-ci.
The great chain of copy-pasta :p

As a sidenote: that scipy moved to meson was the final motivator for us to dare the jump to meson too.

@dnicolodi
Copy link
Member Author

dnicolodi commented Sep 4, 2022

@rgommers @eli-schwartz Thank you for having a look. The whole meson.build looks like this:

project(
  'python-siphash24', 'c', 'cython',
  version: '1.0.0',
  license: 'Apache-2.0 or LGPL-2.1-or-later',
  meson_version: '>= 0.62.0',
  default_options: [
    'buildtype=release',
    'c_std=c11',
  ]
)

py_mod = import('python')
py = py_mod.find_installation()
py_dep = py.dependency()

# MinGW-W64
if host_machine.system() == 'windows' and meson.get_compiler('c').get_id() == 'gcc'
  add_project_link_arguments('-lucrt', '-static', language: 'c')
  add_project_arguments('-mlong-double-64', language: 'c')
  add_project_arguments('-DMS_WIN64', language: 'c')
endif

py.extension_module(
  'siphash24', 'siphash24.pyx', 'c-siphash.c',
  install: true,
  dependencies: py_dep,
)

thus debug=false is implied, I think. Originally I copied buildtype=debugoptimized from the SciPy 'smeson.build but I changed it to buildtype=release after seeing the problem with the debug symbols. Both settings result in the same error. I don't have a Windows machine I can use for testing, I rely on Github Actions runners.

@rgommers I have the impression that the error comes from meson install --prefix .... as run by meson-python. Later down in the build logs I see meson-python trying to copy the .pdb file into the wheel and failing, thus it seem that meson and meson-python agree on which file should have been generated.

@eli-schwartz
Copy link
Member

I don't know too much about how pdb works on Windows. I do know that Meson marks both pdb and implib specially in the pickled installdata with a t.optional attribute, and as a result Meson will try to install it if it exists but will otherwise verbosely log that it doesn't exist and is being ignored.

Note: for implib, which you need in order to link to the ABI of a shared object, only the implib for shared modules as those aren't necessarily linked to or provide an ABI -- regular dlls are presumed to have an ABI, and need an implib or they are useless (generally, they are buggy due to not dllexporting anything).

When, exactly, a pdb is not built, is a question I cannot answer.

@lithomas1
Copy link
Member

lithomas1 commented Sep 9, 2022

FYI, I was looking into #10639, as part of my work on moving pandas to meson.

After some investigating...

Looks like in debug mode we are passing "/Zi" to MSVC's cl.exe which makes the pdb file. (Not too sure though)
However, a target is still created in meson for the pdb file even when we are not in debug mode.
See here,

if not should_strip and t.get_debug_filename():
debug_file = os.path.join(self.get_target_dir(t), t.get_debug_filename())
i = TargetInstallData(debug_file, first_outdir,
first_outdir_name,
False, {}, set(), '',
install_mode, t.subproject,
optional=True, tag='devel')
d.targets.append(i)

Now, look at where t.get_debug_filename is defined. In the case of SharedLibrary, we have the code here.

meson/mesonbuild/build.py

Lines 2144 to 2149 in 167356a

elif self.get_using_msvc():
# Shared library is of the form foo.dll
prefix = ''
# Import library is called foo.lib
self.import_filename = self.vs_import_filename
create_debug_file = True
. This code is very suspicious, since it assumes the creation of a pdb file even when one may not be created.

We see here,

meson/mesonbuild/build.py

Lines 2208 to 2209 in 167356a

if create_debug_file:
self.debug_filename = os.path.splitext(self.filename)[0] + '.pdb'

that meson just hardcodes in the pdb path without even checking whether it should exist, when compiler == MSVC

Now, this doesn't really matter, since ninja(???) I assume is just ignore files to install when they exist.

@eli-schwartz Can you confirm if I'm on the right track? Thanks.

This may be all wrong, so sorry if I'm just spewing noise.

My fix locally does seem to work on a release build, but I haven't tested a debug build to make sure those are still generated.
I also have no clue where and how to add tests for this.

@lithomas1
Copy link
Member

Also, slightly off-topic:
@rgommers Do you know if scipy is also hitting this in their Windows meson builds?

I feel like I'm hitting a lot of sharp edges trying to get meson to work with pandas, which makes me suspect I'm doing something wrong.

@rgommers
Copy link
Contributor

rgommers commented Sep 9, 2022

@lithomas1 no we aren't, I have never seen this. However, we are building with Mingw-w64. So it being an MSVC-specific flag would make sense here.

I feel like I'm hitting a lot of sharp edges trying to get meson to work with pandas, which makes me suspect I'm doing something wrong.

I don't think you are doing something wrong, it is mostly a case of both meson and meson-python being immature for the specific purpose of Python packaging. SciPy was the first major adopter with cross-platform needs, before it was mostly smaller Python needs and a lot of that Linux-specific is my impression. I have run Meson in CI for SciPy on my own fork since mid-2021 and since the start of 2022 in the main SciPy repo. Typically when something is fixed, it stays fixed - because Meson has good test coverage. However, I am right now (after the SciPy 1.9 release which defaults to Meson when building from source) also finding lots of papercuts, things that don't work with only PyPy, or only on Gentoo, or with MSVC. Most fixes are merged or in flight, but I believe it will only really work smoothly with the next meson and meson-python releases.

Just to be sure: with this pdb issue you're only seeing a warning, right? Or is your build failing?

@dnicolodi
Copy link
Member Author

Just to be sure: with this pdb issue you're only seeing a warning, right? Or is your build failing?

It is a warning from meson but meson-python expects the file to be there and crashes when it does not find it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants