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

Improve Testing For Importing Multiple Extension Modules From One File (and Deal With _testimportmultiple.c) #124978

Open
ericsnowcurrently opened this issue Oct 4, 2024 · 2 comments
Labels
3.14 new features, bugs and security fixes extension-modules C modules in the Modules dir tests Tests in the Lib/test dir

Comments

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Oct 4, 2024

(low priority)

The capability of loading multiple extension modules from a single .so file was added over a decade ago. AFAIK the feature wasn't actually tested at all for several years. The PEP 489 implementation added some test coverage, but only indirectly, superficially, and (perhaps) unintentionally. Even then, the feature wasn't tested for single-phase init modules until a couple years ago and, again, only indirectly, superficially, and unintentionally. (FWIW, I'm not sure the feature is meaningfully documented at all.)

Here's the related history:

(expand)
  • 2012 (3.4) - Modules/_testimportmultiple.c added (6b2cbeb); not actually used?
  • 2015 (3.5) - Modules/_testmultiphase.c added for PEP 489 (d5cacbb); contained multiple modules (like `_testimportmultiple), but actually used in tests
  • 2022 (3.12) - Modules/_testsinglephase.c added (gh-99039); so we could stop using the _testcapi module to exercise singlephase init in tests; contained the impl. of the one module
  • 2023 (3.12) - Modules/_testsinglephase.c now implements multiple extension modules (gh-101891)

At the time we added the feature originally, we also added an example implementation: Modules/_testimportmultiple.c. However, it looks like it hasn't ever actually been used in any tests. (Perhaps I'm missing something.) We should use it and its modules to test the feature, or drop the file.

With all that in mind, we should consider improving test coverage of the feature, and use _testimportmultiple.c (or drop it).

To be clear, sorting this out is fairly low priority. We have basic coverage of the feature, I'm not sure it's very widely used, and the status quo isn't causing any problems.

(Honestly, I've created this issue only because I noticed _testimportmultiple.c isn't used anywhere and because of how clunky tests using _testsinglephae.c and _testmultiphase.c are to both understand and to add. However, while looking closer, I realized the feature wasn't explicitly tested, nor thoroughly.)


Here's how I think we should proceed:

  1. determine what aspects of the feature need to be tested (see below)
  2. create the new Modules/_testimportmultiple directory
  3. for each test case:
    a. implement the test (e.g. in Lib/test/test_import/__init__.py or Lib/test/test_import/test_ext_multiple_in_file.py)
    b. add the new .c file (see the devguide) under Modules/_testimportmultiple/
    c. implement the relevant modules there
  4. delete Modules/_testimportmultiple.c

We should be sure the following is tested:

  • multiphase-init
    • 2 modules in the same file (1 direct, 1 indirect)
      • load direct only
      • load indirect only
      • load direct then indirect
      • load indirect then direct
    • 3 modules in the same file (1 direct, 2 indirect)
      • similar permutations, including both indirect-only, indirect-indirect-only, indirect-direct-indirect, etc.
    • 1 indirect module without a direct one (init func name does not match filename
  • singlephase-init
    • same as multi-phase init
    • 2 modules in same file that share some non-exported state stored in a global variable
  • singlephase-init and multiphase-init modules in the same file

We should be sure that the new tests include whatever _testsinglephase.c and _testmultiphase.c currently cover specific to multiple-modules-in-a-file.

The modules for each case could all be in a single file, i.e. the existing Modules/_testimportmultiple.c, but it probably makes more sense to implement each case in its own file (in a new Modules/_testimportmultiple directory).


Again, the feature is actually tested currently, albeit indirectly and only superficially, through Modules/_testmultiphase.c and Modules/_testsinglephase.c. However, as far as I know, there isn't any need for the multiple-modules-in-one-file approach in either case. Once we're directly testing the feature using _testimportmultiple.c, we can stop testing it indirectly with the other two. See gh-124983.

@ericsnowcurrently ericsnowcurrently added tests Tests in the Lib/test dir extension-modules C modules in the Modules dir 3.14 new features, bugs and security fixes labels Oct 4, 2024
@ericsnowcurrently ericsnowcurrently changed the title Test Importing Multiple Extension Modules From One File Using Modules/_testimportmultiple.c Test Importing Multiple Extension Modules From One File Using Modules/_testimportmultiple.c And Expand Test Coverage Oct 4, 2024
@ericsnowcurrently ericsnowcurrently changed the title Test Importing Multiple Extension Modules From One File Using Modules/_testimportmultiple.c And Expand Test Coverage Improve Testing For Importing Multiple Extension Modules From One File (and Deal With _testimportmultiple.c) Oct 4, 2024
@ericsnowcurrently
Copy link
Member Author

CC @encukou

@encukou
Copy link
Member

encukou commented Oct 7, 2024

how clunky tests using _testsinglephase.c and _testmultiphase.c are to both understand and to add

This piece of the analysis surprised me a bit. I thought we're just missing a convenient helper here; generally I'd prefer multiple PyInit* over having to deal with multiple .so/.dll files.

[Modules/_testimportmultiple.c] hasn't ever actually been used in any tests

It has: test_imp.test_issue16421_multiple_modules_in_one_dll was removed last year, when the imp module was removed in #98573 :(
Perhaps there are more tests in there that ought to be resurrected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.14 new features, bugs and security fixes extension-modules C modules in the Modules dir tests Tests in the Lib/test dir
Projects
None yet
Development

No branches or pull requests

2 participants