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

Meson: detect changes in .pxd .pyx .pxi files #29

Closed
noDGodiaev opened this issue Jul 23, 2021 · 17 comments · Fixed by #35
Closed

Meson: detect changes in .pxd .pyx .pxi files #29

noDGodiaev opened this issue Jul 23, 2021 · 17 comments · Fixed by #35
Labels
bug Something isn't working build issues

Comments

@noDGodiaev
Copy link
Collaborator

noDGodiaev commented Jul 23, 2021

I want syntax error checks and changes detection in .pxd files from meson during the developing process.
For my experiment, I choose rndm files.
I use that sequence of commands:

meson setup build --prefix=$PWD/installdir
meson compile -C build
meson install -C build

Current build:

_rndm_cpp = custom_target('rndm_cpp',
  output : 'rndm.cpp',
  input : 'rndm.pyx',
  command : [cython, '--cplus', '-3',
       '--fast-fail', '@INPUT@', '-o', '@OUTPUT@']
)
  1. Add new line: print('some useful content') at rndm.pxd
    setup > Directory already configured.
    compile > ninja: no work to do.
    install: do the installation

Result: I get new line in installdir/.../rndm.pxd

  1. Add new line with syntax error at rndm.pxd
    setup > Directory already configured.
    compile > ninja: no work to do.
    install: do the installation

Result: I get new line with syntax error in installdir/.../rndm.pxd

  1. Add new line: with syntax error at rndm.pyx
    setup > Directory already configured.
    compile > Error compiling Cython file
@noDGodiaev
Copy link
Collaborator Author

noDGodiaev commented Jul 23, 2021

Change .cpp compilation -- add .pxd into it

_rndm_cpp = custom_target('rndm_cpp',
  output : 'rndm.cpp',
  input : ['rndm.pyx', 'rndm.pxd',],
  command : [cython, '--cplus', '-3',
      '--fast-fail', '@INPUT@', '-o', '@OUTPUT@']
)

Try without adding syntax errors:

setup: passed
compile:

from numpy.random cimport bitgen_t

cdef class RndmWrapper():
    cdef:
        double[::1] buf
                   ^
------------------------------------------------------------
/home/casper/PycharmProjects/test_meson_pxds/mc_lib/rndm.pxd:7:20:
C attributes cannot be added in implementation part of extension 
type defined in a pxd
ninja: build stopped: subcommand failed.

@noDGodiaev
Copy link
Collaborator Author

Change .so generating -- add .pxd into it

py3.extension_module(
  'rndm', [_rndm_cpp, 'rndm.pxd'],
  include_directories: inc_np,
  dependencies : py3_dep,
  install: true,
  subdir: install_dir
)

Try without adding syntax errors:
setup:

ERROR: No specified compiler can handle file mc_lib/rndm.pxd

@ev-br
Copy link
Owner

ev-br commented Jul 23, 2021

So is there a way of making meson detect changes in.pxd and .pxi files, however clunky? If there is, let's use it and talk to meson devs with the current way and a desired way.

@ev-br
Copy link
Owner

ev-br commented Jul 23, 2021

BTW, how does meson handle C header files? I cannot believe it does not detect changes in those

@noDGodiaev
Copy link
Collaborator Author

Add .pxd as dependency

_rndm_pxd = custom_target('_rndm_pxd',
  output : 'rndm.pxd',
  input : 'rndm.pxd',
  command : ['cp', '@INPUT@', '@OUTDIR@'],
)
cython_rndm_pxd = declare_dependency(sources: _rndm_pxd)

py3.extension_module(
  'rndm', _rndm_cpp,
  include_directories: inc_np,
  dependencies : [py3_dep, cython_rndm_pxd],
  install: true,
  subdir: install_dir
)
)

Try without adding syntax errors:
setup: passed
compile: passed
install: passed

The same result, as with the current build

@noDGodiaev
Copy link
Collaborator Author

noDGodiaev commented Jul 23, 2021

BTW, how does meson handle C header files? I cannot believe it does not detect changes in those

../mc_lib/_observable/observable.h:29:5: error: ‘terrible’ does not name a type
   29 |     terrible mistake
      |     ^~~~~~~~

Caught it
I suppose it's because of cdef extern ...

@noDGodiaev
Copy link
Collaborator Author

I found a solution:

You need to use

meson setup build --wipe \ --reconfigure

Flags:--wipe to rebuild or --reconfigure to check changes.

Example:

  1. Build without errors (setup>compile>install)

  2. Add some errors in rndm.pxd:

    # implementation details
    cdef void _fill(self) nogil
    terrible error
  1. Repeat all stages: (setup>compile>install). Especially for setup use
    And... at compile stage error is caught
FAILED: mc_lib/rndm.cpp 
/home/casper/PycharmProjects/envs/test_meson_pxds/bin/cython --cplus -3 --fast-fail ../mc_lib/rndm.pyx -o mc_lib/rndm.cpp

Error compiling Cython file:
------------------------------------------------------------
...
    cdef double uniform(self) nogil
 
    # implementation details
    cdef void _fill(self) nogil

    terrible error   ^
------------------------------------------------------------

@ev-br
Copy link
Owner

ev-br commented Jul 23, 2021

Can one always use one of these flags? (Both less to memorize and easier to script the dev workflow)

@noDGodiaev
Copy link
Collaborator Author

No, both flags seeking the named folder, where build is. So for the first launch of the setup must be with no compilation flags.

@ev-br ev-br changed the title Meson: detect changes in .pxd .pyx .pix files Meson: detect changes in .pxd .pyx .pxi files Jul 23, 2021
@rgommers
Copy link

I think that's related to (or the same as) mesonbuild/meson#8961. I haven't yet found the time to dig in more, but it should be fixable. Basically the problem is that you can pass anything to dependency: , and Meson only adds Ninja rules for specific file types. .pxi and .pxd are Cython-specific, so the Cython support in Meson needs to be updated for them.

@ev-br
Copy link
Owner

ev-br commented Jul 25, 2021

ISTM it's somewhat simpler--- pyx/pxd files are just like h/cpp pairs, which are tracked by meson, right?

@rgommers
Copy link

Well Meson emitsbuild.ninja, and the dependency must be there - it seems missing.

@ev-br
Copy link
Owner

ev-br commented Jul 25, 2021

So I take it it's an meson upstream ticket material?

@rgommers
Copy link

Oh one thing to check first, normally dependencies get tracked automatically, but not for custom_target I believe. So this is how the example should look:

_rndm_cpp = custom_target('rndm_cpp',
  output : 'rndm.cpp',
  input : 'rndm.pyx',
  depend_files: 'rndm.pxd',  # probably this is it
  command : [cython, '--cplus', '-3',
       '--fast-fail', '@INPUT@', '-o', '@OUTPUT@']
)

See the description of depend_files in https://mesonbuild.com/Reference-manual.html#custom_target.

@ev-br
Copy link
Owner

ev-br commented Jul 25, 2021

Bingo! gh-35 seems to do it. Steps to reproduce

$ git clean -xdf
$ meson setup build -C installdir
$ meson compile -C build
$ ... add a syntax error to rndm.pxd or observable.pxd
$ meson compile -C build

This passes on master and fails with gh-35.

Somehow breaking _observable/observable.h in a similar way already breaks down on master, as Dmitry was saying before, so there is some magic for .h files already. No idea. The whole way _observable/observable.h is handled at https://github.com/ev-br/mc_lib/blob/master/mc_lib/_observable/meson.build is somewhat hacky --- it's certainly not a python source file, but it needs installing I guess.

@ev-br ev-br added the bug Something isn't working label Jul 25, 2021
@rgommers
Copy link

so there is some magic for .h files already. No idea.

This should explain it: https://ninja-build.org/manual.html#ref_headers.

it's certainly not a python source file, but it needs installing I guess.

python_sources is just a variable name. You can write it as:

py3.install_sources(
    'observable.h',
    pure: false,
    subdir: 'mc_lib/_observable'
)

which means install some source file install a "python install".

There are other general Meson commands like install_data, but they don't know about Python-specific paths like site-packages.

@ev-br ev-br closed this as completed in #35 Jul 25, 2021
@ev-br
Copy link
Owner

ev-br commented Jul 25, 2021

Thanks Ralf!
I'm a bit out if my depth with the ninja docs, but it's great to have a reference for the day it's needed. (Am hoping for an if but suspect it's a when)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants