Skip to content

TypeOffset class no longer depends on target Python version #1292

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 30, 2020
Merged

TypeOffset class no longer depends on target Python version #1292

merged 4 commits into from
Nov 30, 2020

Conversation

lostmsu
Copy link
Member

@lostmsu lostmsu commented Nov 24, 2020

This is a stage of removing the need to prepare a separate build of Python.Runtime for each Python version.

What does this implement/fix? Explain your changes.

This removes the need to pick TypeOffset at compile time. Instead, for each supported Python version a separate class is generated (e.g. TypeOffset36). Then the Runtime (via new class ABI) picks the correct class based on actual Python version using reflection, and copies only the necessary TypeOffset members over from it.

ManagedDataOffsets.Magic is now also read at runtime from tp_basicsize of the builtins.type class.

Any other comments?

N/A

Checklist

Check all those that are applicable and complete. N/A

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@lostmsu lostmsu requested a review from filmor November 24, 2020 08:32
filmor
filmor reviewed Nov 24, 2020
View reviewed changes
filmor
filmor reviewed Nov 24, 2020
View reviewed changes
tools/geninterop/geninterop.py Outdated
@@ -351,6 +350,10 @@ def main():
gen_interop_head(writer)

gen_heap_type_members(ast_parser, writer)

ver_define = "PYTHON{0}{1}".format(PY_MAJOR, PY_MINOR)
writer.append(code="#if " + ver_define)
Copy link
Member

Choose a reason for hiding this comment

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

Why are these still behind a compile-time flag? Do you plan to change that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I am approaching this gradually.

Copy link
Member Author

@lostmsu lostmsu Nov 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I checked this out, and the rest of the conditional stuff came from 1b466df. Since it was only used to determine if methods in Python.Runtime itself implement slots (which we can do on our own), I took a liberty to mostly revert it in this PR. Reversal is in this commit

@lostmsu lostmsu force-pushed the features/VersionIndependent branch 3 times, most recently from d6f4834 to 3bc282a Compare November 25, 2020 02:25
@codecov-io
Copy link

codecov-io commented Nov 25, 2020 •
edited
Loading

Codecov Report

Merging #1292 (f9a3a53) into master (182faed) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1292   +/-   ##
=======================================
  Coverage   87.97%   87.97%           
=======================================
  Files           1        1           
  Lines         291      291           
=======================================
  Hits          256      256           
  Misses         35       35
Flag Coverage Δ
setup_linux 65.29% <ø> (ø)
setup_windows 74.22% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 182faed...f9a3a53. Read the comment docs.

@lostmsu
Copy link
Member Author

lostmsu commented Nov 25, 2020

Hm, Travis build on Linux with Python 3.9 regularly fails with this change. Could I have screwed up something here? @filmor, you've recently added support for 3.9. Was there anything specific to Linux?

@filmor
Copy link
Member

filmor commented Nov 25, 2020

Nope, the only reason why support for 3.9 took so long was that I had to adjust the interop generation as it wasn't working with the new header-files anymore.

@lostmsu lostmsu closed this Nov 25, 2020
@lostmsu lostmsu reopened this Nov 25, 2020
@lostmsu
Copy link
Member Author

lostmsu commented Nov 25, 2020 •
edited
Loading

@filmor , oh, nvm. Looks like Travis just used older commit for 3.9 o-O

It passes now.

@lostmsu lostmsu requested a review from filmor November 28, 2020 22:04
filmor
filmor reviewed Nov 29, 2020
View reviewed changes
Copy link
Member

@filmor filmor left a comment

Choose a reason for hiding this comment

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

I think this is fine for now, just minor things that I think could be fixed.

@lostmsu lostmsu force-pushed the features/VersionIndependent branch from 3bc282a to a2c13c7 Compare November 29, 2020 22:08
@lostmsu
TypeOffset class no longer depends on target Python version
bbc0dfb 
Instead, for each supported Python version a separate class is generated (e.g. TypeOffset36). Then the Runtime picks the correct class using reflection, and copies only the necessary TypeOffset members over from it.

ManagedDataOffsets.Magic is now also read at runtime from tp_basicsize of PyType
@lostmsu
implemented NativeTypeOffset for installation with setup.py
6dd92ad
@lostmsu
fixed missing ITypeOffset members np_inplace_*
e36a027
@lostmsu lostmsu force-pushed the features/VersionIndependent branch from a2c13c7 to e36a027 Compare November 29, 2020 22:09
@lostmsu lostmsu requested a review from filmor November 29, 2020 22:48
@lostmsu lostmsu merged commit ab97b02 into pythonnet:master Nov 30, 2020
@lostmsu lostmsu deleted the features/VersionIndependent branch November 30, 2020 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filmor filmor Awaiting requested review from filmor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lostmsu @codecov-io @filmor