Structured Sources

[dcbaker]

This is a reworked version of #8775, with the main complaints resolved.

The purpose of this series is to provide a mechanism for languages in which filesystem layout is relevant to ensure that generated sources end up in the right place. To do this, one describes the layout they want to a structrued_sources object, which the backend converts (if necessary) into a series of copy commands, to build the correct directory structure in the build directory. This structure is placed in the private directory of the target, to eliminate collisions. Then the actual compile is done on the copied sources rather than on the original sources. This is done at build time to avoid reconfiguration when any of the structured sources are changed.

[dcbaker]

@rgommers I'd like to have this work for cython too before it's merged, but I'm struggling to write a test case so that I can get started. Do you think you could at least help point me in the right direction?

[codecov]

Codecov Report

Merging #9339 (3f3602e) into master (ade6e3a) will decrease coverage by 2.80%.
The diff coverage is n/a.

❗ Current head 3f3602e differs from pull request most recent head ff4c283. Consider uploading reports for the commit ff4c283 to get more accurate results

@@            Coverage Diff             @@
##           master    #9339      +/-   ##
==========================================
- Coverage   64.66%   61.85%   -2.81%     
==========================================
  Files         402      202     -200     
  Lines       85729    42858   -42871     
  Branches    18909     9374    -9535     
==========================================
- Hits        55433    26509   -28924     
+ Misses      25934    14229   -11705     
+ Partials     4362     2120    -2242     

Impacted Files	Coverage Δ
scripts/msgfmthelper.py	83.33% <0.00%> (-16.67%)	⬇️
modules/unstable_wayland.py	75.00% <0.00%> (-3.19%)	⬇️
modules/i18n.py	76.37% <0.00%> (-2.99%)	⬇️
scripts/gettext.py	37.50% <0.00%> (-2.80%)	⬇️
cmake/traceparser.py	55.46% <0.00%> (-1.60%)	⬇️
scripts/itstool.py	66.00% <0.00%> (-0.67%)	⬇️
build.py	75.21% <0.00%> (-0.33%)	⬇️
minstall.py	67.61% <0.00%> (-0.33%)	⬇️
mintro.py	90.25% <0.00%> (-0.19%)	⬇️
interpreter/dependencyfallbacks.py	94.71% <0.00%> (-0.11%)	⬇️
... and 212 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8434fb1...ff4c283. Read the comment docs.

[rgommers]

@rgommers I'd like to have this work for cython too before it's merged, but I'm struggling to write a test case so that I can get started. Do you think you could at least help point me in the right direction?

Yes of course. There are three file types that matter: __init__.py's and .pxi/.pxd files. Here is a relevant subset of the SciPy code with comments:

scipy/meson.build:

# Needed to trick Cython, it won't do a relative import outside a package.
# Every Cython extension built from a generated `.pyx` file needs to rely on this,
# as well as on the `__init__.py` files in subdirs between the root one we're copying
# here and the subdir it's in.
_cython_tree = custom_target('_cython_tree',
  output : [
    '__init__.py',
    'linalg.pxd',
    'special.pxd',
  ],
  input : [
    '__init__.py',
    'linalg.pxd',
    'special.pxd',
  ],
  command : ['cp', '@INPUT@', '@OUTDIR@'],
)

Codegen in scipy/special/meson.build:

cython_special = custom_target('cython_special',
  output : [...
            '_ufuncs.pyi',
            'cython_special.pyx',
            'cython_special.pxd'
           ],
  input : ['_generate_pyx.py', 'functions.json', 'add_newdocs.py'],
  command : [py3, '@INPUT0@', '-o', '@OUTDIR@'],
)

scipy/stats/meson.build (uses cython_special):

# Relies on cython_special, which ends up in the build dir. Hence we need ` '-I', '@BUILD_ROOT@'` here.
# (note: I need to double check if this `custom_target` is still needed, may have to do with the now
#        solved bug about `cython_special` not being indexable when passed to `extension_module`.
#        Either that, or to allow using `@BUILD_ROOT@` here.)
_stats_c = custom_target('_stats_c',
  output: '_stats.c',
  input: [
    '_stats.pyx',
    _cython_tree,
    cython_special,
  ],
  command: [cython, '-3', '--fast-fail',
            '@INPUT0@', '-o', '@OUTPUT@', '-I', '@BUILD_ROOT@'],
)

_stats = py3.extension_module('_stats',
  _stats_c,
  dependencies : [py3_dep],
  install : true,
  subdir : 'scipy/stats'
)

The __init__.py files should always be copied over I believe (without listing them in structured sources), and that logic is easy to implement. The .pxd/.pxi files need to be explicitly declared I'd think.

I believe the above covers most/all of what is relevant. Please let me know if it's not clear or if I should try and turn this into a self-contained test case.

EDIT: the bug in gh-8961 is relevant to this.

[dcbaker]

@rgommers: I've added a cython test, and it compiles but doesn't run. Maybe you can help me, I'm bashing my head against the wall, the merge window closes Sunday, and I'd really like to get this in before that.

[eli-schwartz]

You have created:

builddir
├── lib.cpython-310-x86_64-linux-gnu.so
└── lib.cpython-310-x86_64-linux-gnu.so.p
    ├── meson-generated_structured_gen_gen.pyx.c.o
    ├── meson-generated_structured_lib.pyx.c.o
    └── structured
        ├── gen
        │   └── gen.pyx
        ├── lib.pyx
        └── lib.pyx.c

Then you add builddir to the PYTHONPATH, run a script that imports lib, which works, and then lib itself tries to do from gen import generated however gen.cpython-310-x86_64-linux-gnu.so is never created. The object file built from gen.pyx is being merged into the lib extension_module.

It seems like the problem is that the structured source is being treated as a file layout for compiling lib, when what you actually want is a file layout for compiling both lib and gen as well as syncing an entire tree of pure python source code into a directory not named *.p/ suitable for public access.

[dcbaker]

So, for the moment I've dropped cython support, and I'll go at that again in a follow up, as it's going to require some thought on how it's going to work, and I'm not sure what that needs to look like.

[jpakkane]

As this is a notable feature that has been long underway, I'll grant an exception so that this can be merged after the actual DL in case it can't get reviewed in time.

[jpakkane]

Please explain what "root" means here, specifically. It is a term with many potential meanings.

[dcbaker]

Oops, that's more leftover old implementation, '' is not allowed in the dictionary. I've fixed that and updated the 0.60 comment.

[jpakkane]

Featurewise this seems like a useful addition so approval on that front.

I have not looked too deeply in the implementation yet, but it seems that there should be a failing test case when using two different structured sources that define the same file in two different structured sources. Something like:

executable(...,
    structured_sources({'subdir': 'clashing.rs'}),
    structured_sources({'subdir': 'other/clashing.rs'})
)

Either there can only be one structured sources per target (which seems a bit limiting) or it should validate all the files upfront.

[dcbaker]

Only one structured_source is allowed to be passed, but I can add a test to ensure that passing two structured_sources fails.

Actually, that's interesting, yeah. I can fix that.

[dcbaker]

@jpakkane I've added the validation and tests you wanted