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

RFC: Include python3.def itself in the Rust source #10

Merged
merged 2 commits into from
Apr 29, 2022

Conversation

adamreichold
Copy link
Member

@adamreichold adamreichold commented Apr 29, 2022

All parsing and processing is done in the CI so that the crate itself includes the python3.def file itself instead of creating it on the fly.

Based on the idea voiced in #9 (comment)

I did try to keep the existing semantics in place and just moved things around to a Python script running in the CI. I just chose Python because I assume that the ubuntu-latest image contains a reasonably up-to-date version. (The updated CI workflow is untested, but I used the script locally the produce the definition file committed here.)

One could try using the upstream script to do the parsing and generating, but I think that should be evaluated separately.

@ravenexp
Copy link
Collaborator

I agree with this change in principle, but I'd like to see at least some test coverage to ensure that the number of Stable ABI exports is non-decreasing with each definitions update PR. The Rust version had something like that.

You should also update LICENSE to remove all mentions of Python sources.

@adamreichold
Copy link
Member Author

I agree with this change in principle, but I'd like to see at least some test coverage to ensure that the number of Stable ABI exports is non-decreasing with each definitions update PR. The Rust version had something like that.

Added this as an assert whenever the parser script is run.

You should also update LICENSE to remove all mentions of Python sources.

Done.

@adamreichold
Copy link
Member Author

Also note that the automatically created PR will contain a diff of the generated definitions file this way, so we can also review that before merging.

@encukou
Copy link

encukou commented Apr 29, 2022

The switch to TOML will happen today or early next week.

Copy link
Collaborator

@ravenexp ravenexp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore this.

Copy link
Collaborator

@ravenexp ravenexp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

LICENSE Outdated Show resolved Hide resolved
All parsing and processing is done in the CI so that the crate itself includes
the python3.def file itself instead of creating it on the fly.
@adamreichold
Copy link
Member Author

The switch to TOML will happen today or early next week.

Thanks for the heads-up! I think we could do import toml then without adding any dependencies to the Rust build scripts. (Or do something simpler, I just mean that we unrestricted w.r.t. our dependency closure.)

@ravenexp
Copy link
Collaborator

The switch to TOML will happen today or early next week.

Thanks for updating us on this. I'll delay the merge until the update script migrates to the TOML source.

@ravenexp ravenexp requested a review from messense April 29, 2022 13:22
@encukou
Copy link

encukou commented Apr 29, 2022

CPython just switched to TOML.

If you stick with Python for parsing, beware that toml is unmaintained/outdated.
Consider tomli, a little, correct TOML parser, which is being added into the Python 3.11 standard library as tomllib. (Full disclosure: I helped)

If you happen to know another project that needs a python3.def (or could use it), please let me know. I could probably generate it and keep it updated in CPython itself to reduce duplication of effort.

@ravenexp
Copy link
Collaborator

If you happen to know another project that needs a python3.def (or could use it), please let me know. I could probably generate it and keep it updated in CPython itself to reduce duplication of effort.

Does this mean that python3.def will be committed to the CPython project repository and we will be able to pull it in as is without generating it ourselves from the TOML definitions?

@encukou
Copy link

encukou commented Apr 29, 2022

Yes. CPython has some infrastructure to generate files based on stable_abi.toml, one more shouldn't be too much of a burden (though other devs might disagree).
A possible issue is that it wouldn't be tested in CPython, so we'd rely on you to report any issues.

Python generates and uses python3dll.c instead. (Hm, would you be able to use that by any chance? I know next to nothing about building on Windows...)
edit: The exact formatting is as unstable as stable_abi.toml, it's only better if you can feed it to a C compiler directly.

@ravenexp
Copy link
Collaborator

Python generates and uses python3dll.c instead. (Hm, would you be able to use that by any chance? I know next to nothing about building on Windows...) edit: The exact formatting is as unstable as stable_abi.toml, it's only better if you can feed it to a C compiler directly.

No, I think it's specific to the Microsoft C++ compiler, and we also support MinGW and LLVM tools (without Clang). Also we only need the import library, not the DLL file itself.

Copy link
Collaborator

@ravenexp ravenexp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

@ravenexp
Copy link
Collaborator

I think we can merge this now (?). I'll wait for @messense to take a look.

@adamreichold
Copy link
Member Author

adamreichold commented Apr 29, 2022

Had to switch to installing python3-pip via apt-get and tomli via pip as ubuntu-latest is 20.04 which does not yet contain the python3-tomli package introduced in 22.04.

@ravenexp
Copy link
Collaborator

Had to switch to installing python3-pip via apt-get and tomli via pip as ubuntu-latest is 20.04 which does not yet contain the python3-tomli package introduced in 22.04.

TBH I was surprised that Ubuntu had up to date Python packages, but I'm mostly using official Rust docker images based on Debian Buster, so pip install was my go-to choice.

Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@messense messense merged commit b714956 into main Apr 29, 2022
@messense messense deleted the include-python3-def branch April 29, 2022 20:35
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 this pull request may close these issues.

4 participants