Add macro append_to_inittab (issue #2359) #2377

herquan · 2022-05-16T04:02:00Z

Sometimes we need to debug in a real environment with our module installed. append_to_inittab will be a wrapper for PyImport_AppendInittab (https://docs.python.org/3/c-api/import.html#c.PyImport_AppendInittab) and help us to do this

davidhewitt

Nice! Much simpler than the proc macro, I'm glad the refactoring seems to have helped :)

Couple of comments. Also please add a CHANGELOG entry.

src/macros.rs

davidhewitt · 2022-05-16T05:06:23Z

tests/test_module.rs

+#[test]
+fn test_module_append_to_inittab() {
+    use pyo3::append_to_inittab;
+    append_to_inittab!(module_with_functions);


You will probably find that this needs to go in its own test file, because it's quite likely that another test will already have initialised the Python interpreter.

mejrs

Thanks :)

Can you please add a test in https://github.com/PyO3/pyo3/blob/main/src/test_hygiene/misc.rs?

mejrs · 2022-05-16T11:46:25Z

src/macros.rs

+/// Add current module to the existing table of built-in modules.
+///
+/// Use it before [`prepare_freethreaded_python`](crate::prepare_freethreaded_python) and
+/// leave feature `auto-initialize` off


To be honest, I wasn't aware of this function, and I think the vast majority of people reading this documentation are not either. So I'd like if the documentation was fleshed out some more.

In particular, a reader might have the following questions or thoughts:

I want to add a module, this is what I need (is it?)

What does this do and why should I use it?

What is the point of adding builtin modules?

You said you use this to debug modules, can you explain? How can a user use this for debugging?

An example might be nice :)

Added here and building_and_distribution.md

herquan · 2022-05-17T02:48:20Z

Thanks :)

Can you please add a test in https://github.com/PyO3/pyo3/blob/main/src/test_hygiene/misc.rs?

I'm not sure what do you want me to add in this test? Do you want test_module_append_to_inittab to be there?

davidhewitt

The purpose of test_hygiene is to ensure that the macros still work even when pyo3 is in unusual import paths (i.e. use ::pyo3 won't work). In test_hygiene, for example, pyo3 is imported as crate.

I think adding the following to the bottom of test_hygiene/misc.rs will be sufficient - the generated code just needs to compile:

#[allow(dead_code)]
fn append_to_inittab(py: crate::Python<'_>) {
    #[crate::pymodule]
    fn my_module(_: crate::Python<'_>, _: &crate::PyModule) -> crate::PyResult<()> {
            ::std::result::Result::Ok(())
    }
    
    crate::append_to_inittab!(my_module);
}

davidhewitt · 2022-05-17T06:30:42Z

tests/test_append_to_inittab.rs

@@ -0,0 +1,32 @@
+#![cfg(feature = "macros")]


Instead of #[cfg] on the test, I think you want:

Suggested change

#![cfg(feature = "macros")]

#![cfg(all(feature = "macros", not(PyPy))]

davidhewitt · 2022-05-17T06:33:45Z

guide/src/building_and_distribution.md

@@ -225,6 +225,10 @@ The known complications are:

 If you encounter these or other complications when linking the interpreter statically, discuss them on [issue 416 on PyO3's Github](https://github.com/PyO3/pyo3/issues/416). It is hoped that eventually that discussion will contain enough information and solutions that PyO3 can offer first-class support for static embedding.

+### Import your module when embedding the CPython interpreter


In general PyPy doesn't support embedding, so I don't think you need to make the distinction here.

Suggested change

### Import your module when embedding the CPython interpreter

### Import your module when embedding the Python interpreter

davidhewitt · 2022-05-17T06:35:59Z

guide/src/building_and_distribution.md

@@ -225,6 +225,10 @@ The known complications are:

 If you encounter these or other complications when linking the interpreter statically, discuss them on [issue 416 on PyO3's Github](https://github.com/PyO3/pyo3/issues/416). It is hoped that eventually that discussion will contain enough information and solutions that PyO3 can offer first-class support for static embedding.

+### Import your module when embedding the CPython interpreter
+
+When you run your Rust binary with CPython (not PyPy) interpreter, your newly created module won't be initialized unless the function defined with macro `#[pymodule]` is added to a table called `PyImport_Inittab`. This means Python statements like `import your_new_module` run by your Rust binary will fail. You can use macro `append_to_inittab` before function `prepare_freethreaded_python` being called to add the module function into that table. Also [`auto-initialize`](features.md#auto-initialize) needs to be turned off in such scenario.


Some phrasing suggestions.

Suggested change

When you run your Rust binary with CPython (not PyPy) interpreter, your newly created module won't be initialized unless the function defined with macro `#[pymodule]` is added to a table called `PyImport_Inittab`. This means Python statements like `import your_new_module` run by your Rust binary will fail. You can use macro `append_to_inittab` before function `prepare_freethreaded_python` being called to add the module function into that table. Also [`auto-initialize`](features.md#auto-initialize) needs to be turned off in such scenario.

When you run your Rust binary with an embedded interpreter, any `#[pymodule]` created modules won't be accessible to import unless added to a table called `PyImport_Inittab` before the embedded interpreter is initialized. This will cause Python statements in your embedded interpreter such as `import your_new_module` to fail. You can call the macro `append_to_inittab` with your module before initializing the Python interpreter to add the module function into that table. (The Python interpreter will be initialized by calling `prepare_freethreaded_python`, `with_embedded_interpreter`, or `Python::with_gil` with the [`auto-initialize`](features.md#auto-initialize) feature enabled.)

Also, it would be great to link append_to_inittab to the docs - see the PYO3_DOCS_URL substitutions we use.

herquan · 2022-05-18T05:39:05Z

The purpose of test_hygiene is to ensure that the macros still work even when pyo3 is in unusual import paths (i.e. use ::pyo3 won't work). In test_hygiene, for example, pyo3 is imported as crate.

I think adding the following to the bottom of test_hygiene/misc.rs will be sufficient - the generated code just needs to compile:
#[allow(dead_code)]
fn append_to_inittab(py: crate::Python<'_>) {
    #[crate::pymodule]
    fn my_module(_: crate::Python<'_>, _: &crate::PyModule) -> crate::PyResult<()> {
            ::std::result::Result::Ok(())
    }
    
    crate::append_to_inittab!(my_module);
}

Macro pymodule alone is not working in this file and it seems there are still some code that needs to handle unusual import path. I think it is reasonable to fix it first before we add such test in this pull request.

davidhewitt · 2022-05-18T05:52:15Z

Macro pymodule alone is not working in this file

My sample above was wrong, it needs to be:

#[allow(dead_code)]
fn append_to_inittab(py: crate::Python<'_>) {
    #[crate::pymodule]
    #[pyo3(crate = "crate")]
    fn my_module(_: crate::Python<'_>, _: &crate::PyModule) -> crate::PyResult<()> {
            ::std::result::Result::Ok(())
    }
    
    crate::append_to_inittab!(my_module);
}

I forgot to tell the #[pymodule] macro what the PyO3 path was!

davidhewitt · 2022-05-18T05:54:26Z

I think it is reasonable to fix it first before we add such test in this pull request.

Please add the test to this PR. If there is an issue elsewhere it will be useful to see CI output anyway.

davidhewitt · 2022-05-19T06:30:49Z

src/test_hygiene/misc.rs

+fn append_to_inittab(py: crate::Python<'_>) {
+    #[crate::pymodule]
+    #[pyo3(crate = "crate")]
+    fn my_module(_: crate::Python<'_>, _: &crate::types::PyModule) -> crate::PyResult<()> {


Ah I see, the problem is that my_module is already used in the other hygiene test. Just need to rename this:

Suggested change

fn my_module(_: crate::Python<'_>, _: &crate::types::PyModule) -> crate::PyResult<()> {

fn module_for_inittab(_: crate::Python<'_>, _: &crate::types::PyModule) -> crate::PyResult<()> {

davidhewitt · 2022-05-19T06:30:57Z

src/test_hygiene/misc.rs

@@ -33,3 +33,13 @@ fn intern(py: crate::Python<'_>) {
    let _foo = crate::intern!(py, "foo");
    let _bar = crate::intern!(py, stringify!(bar));
 }
+
+#[allow(dead_code)]
+fn append_to_inittab(py: crate::Python<'_>) {


My bad.

Suggested change

fn append_to_inittab(py: crate::Python<'_>) {

fn append_to_inittab() {

davidhewitt · 2022-05-19T06:33:22Z

src/macros.rs

+            ::std::assert_eq!(
+                $crate::ffi::Py_IsInitialized(),
+                0,
+                "called `append_to_inittab_impl` but a Python interpreter is already running."


Suggested change

"called `append_to_inittab_impl` but a Python interpreter is already running."

"called `append_to_inittab` but a Python interpreter is already running."

davidhewitt · 2022-05-19T06:36:04Z

src/macros.rs

+                "called `append_to_inittab_impl` but a Python interpreter is already running."
+            );
+            $crate::ffi::PyImport_AppendInittab(
+                concat!(stringify!($module), "\0").as_ptr() as *const ::std::os::raw::c_char,


Ah, just spotted here: instead of concat!(stringify!($module), "\0") this should actually look up the name from the macro's output, because #[pyo3(name = "...")] option means the function ident isn't always the name exposed to Python.

You'll probably need to add const NAME: &'static str to the macro output generated at

pyo3/pyo3-macros-backend/src/module.rs

Line 78 in 4869052

#visibility mod #fnname {

and then can use $module::NAME here.

I don't know about too much fancy grammars in rust and hopefully my new change looks good.

davidhewitt · 2022-05-21T18:28:53Z

src/macros.rs

+macro_rules! append_to_inittab {
+    ($module:ident) => {
+        unsafe {
+            ::std::assert_eq!(


Hmmm, looks like assert_eq is not hygienic until 1.50. @mejrs how do you feel if we just put #[rust version::since(1.50)] on this test and live with it? It's such a niche case that messying the implementation for the older compilers (to support only no_implicit_prelude) doesn't seem worth it.

It can just be if a != b { panic!()} rather than an assert_eq (may need an allow to stop clippy from complaining about it though)

davidhewitt · 2022-05-24T06:20:09Z

I'm going to force-push a formatting fix and then hopefully this can be merged. Thanks @herquan for proposing and implementing this!

Sometimes we need to debug in a real environment with our module installed. `append_to_inittab` will be a wrapper for PyImport_AppendInittab (https://docs.python.org/3/c-api/import.html#c.PyImport_AppendInittab) and help us to do this

herquan changed the title ~~Add macro append_to_inittab~~ Add macro append_to_inittab (issue #2359) May 16, 2022

davidhewitt reviewed May 16, 2022

View reviewed changes

herquan force-pushed the herquan_cr1 branch from a699244 to f9644e0 Compare May 16, 2022 05:41

mejrs requested changes May 16, 2022

View reviewed changes

herquan force-pushed the herquan_cr1 branch 2 times, most recently from b81ca14 to 0b2e831 Compare May 17, 2022 03:16

davidhewitt reviewed May 17, 2022

View reviewed changes

herquan force-pushed the herquan_cr1 branch from b7043f4 to af5a462 Compare May 18, 2022 05:45

herquan force-pushed the herquan_cr1 branch from ad818c3 to 7679bab Compare May 19, 2022 04:59

davidhewitt reviewed May 19, 2022

View reviewed changes

herquan force-pushed the herquan_cr1 branch 2 times, most recently from c5e97e9 to c49947f Compare May 21, 2022 04:47

davidhewitt reviewed May 21, 2022

View reviewed changes

herquan force-pushed the herquan_cr1 branch from 9913f05 to 71c33dc Compare May 23, 2022 03:26

davidhewitt linked an issue May 24, 2022 that may be closed by this pull request

[needs-design]Can we let macro generate function to append module to init table? #2359

Closed

davidhewitt force-pushed the herquan_cr1 branch from 71c33dc to d84607e Compare May 24, 2022 06:21

Add macro append_to_inittab

2ec4773

Sometimes we need to debug in a real environment with our module installed. `append_to_inittab` will be a wrapper for PyImport_AppendInittab (https://docs.python.org/3/c-api/import.html#c.PyImport_AppendInittab) and help us to do this

davidhewitt force-pushed the herquan_cr1 branch from d84607e to 2ec4773 Compare May 24, 2022 06:42

davidhewitt enabled auto-merge May 24, 2022 06:54

davidhewitt merged commit 126bf49 into PyO3:main May 24, 2022

herquan deleted the herquan_cr1 branch May 25, 2022 02:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add macro append_to_inittab (issue #2359) #2377

Add macro append_to_inittab (issue #2359) #2377

herquan commented May 16, 2022

davidhewitt left a comment

davidhewitt May 16, 2022

mejrs left a comment

mejrs May 16, 2022 •

edited

Loading

herquan May 17, 2022

herquan commented May 17, 2022

davidhewitt left a comment

davidhewitt May 17, 2022

davidhewitt May 17, 2022

davidhewitt May 17, 2022

herquan commented May 18, 2022

davidhewitt commented May 18, 2022 •

edited

Loading

davidhewitt commented May 18, 2022

davidhewitt May 19, 2022

davidhewitt May 19, 2022

davidhewitt May 19, 2022

davidhewitt May 19, 2022

herquan May 20, 2022

davidhewitt May 21, 2022

mejrs May 22, 2022

davidhewitt commented May 24, 2022

	#![cfg(feature = "macros")]
	#![cfg(all(feature = "macros", not(PyPy))]

		@@ -225,6 +225,10 @@ The known complications are:

		If you encounter these or other complications when linking the interpreter statically, discuss them on [issue 416 on PyO3's Github](https://github.com/PyO3/pyo3/issues/416). It is hoped that eventually that discussion will contain enough information and solutions that PyO3 can offer first-class support for static embedding.

		### Import your module when embedding the CPython interpreter

	fn my_module(_: crate::Python<'_>, _: &crate::types::PyModule) -> crate::PyResult<()> {
	fn module_for_inittab(_: crate::Python<'_>, _: &crate::types::PyModule) -> crate::PyResult<()> {

	fn append_to_inittab(py: crate::Python<'_>) {
	fn append_to_inittab() {

	"called `append_to_inittab_impl` but a Python interpreter is already running."
	"called `append_to_inittab` but a Python interpreter is already running."

Add macro append_to_inittab (issue #2359) #2377

Add macro append_to_inittab (issue #2359) #2377

Conversation

herquan commented May 16, 2022

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mejrs left a comment

Choose a reason for hiding this comment

mejrs May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herquan commented May 17, 2022

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herquan commented May 18, 2022

davidhewitt commented May 18, 2022 • edited Loading

davidhewitt commented May 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt commented May 24, 2022

mejrs May 16, 2022 •

edited

Loading

davidhewitt commented May 18, 2022 •

edited

Loading