FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

TypeOffset class no longer depends on target Python version by lostmsu · Pull Request #1292 · pythonnet/pythonnet · GitHub

/ pythonnet Public

TypeOffset class no longer depends on target Python version - #1292

Merged
lostmsu merged 4 commits into
pythonnet:masterfrom
losttech:features/VersionIndependent
Nov 30, 2020
ConversationCommits (4)ChecksFiles changed
Merged

TypeOffset class no longer depends on target Python version#1292
lostmsu merged 4 commits into
pythonnet:masterfrom
losttech:features/VersionIndependent

Conversation

lostmsu commented Nov 24, 2020

Copy link
Copy Markdown
Member

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 requested a review from filmor November 24, 2020 08:32
filmor reviewed Nov 24, 2020
View reviewed changes
Comment thread tools/geninterop/geninterop.py Outdated
filmor reviewed Nov 24, 2020
View reviewed changes
Comment thread tools/geninterop/geninterop.py Outdated
gen_heap_type_members(ast_parser, writer)

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

filmor Nov 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lostmsu Nov 24, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, I am approaching this gradually.

lostmsu Nov 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the features/VersionIndependent branch 3 times, most recently from d6f4834 to 3bc282a Compare November 25, 2020 02:25

codecov-io commented Nov 25, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@           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 commented Nov 25, 2020

Copy link
Copy Markdown
Member Author

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 commented Nov 25, 2020

Copy link
Copy Markdown
Member

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 closed this Nov 25, 2020
lostmsu reopened this Nov 25, 2020

lostmsu commented Nov 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

It passes now.

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

filmor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread .gitignore Outdated
Comment thread src/runtime/native/ABI.cs
Comment thread src/runtime/runtime.cs
Comment thread tools/geninterop/geninterop.py Outdated
lostmsu force-pushed the features/VersionIndependent branch from 3bc282a to a2c13c7 Compare November 29, 2020 22:08
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
implemented NativeTypeOffset for installation with setup.py
6dd92ad
fixed missing ITypeOffset members np_inplace_*
e36a027
lostmsu force-pushed the features/VersionIndependent branch from a2c13c7 to e36a027 Compare November 29, 2020 22:09
lostmsu requested a review from filmor November 29, 2020 22:48
lostmsu merged commit ab97b02 into pythonnet:master Nov 30, 2020
lostmsu deleted the features/VersionIndependent branch November 30, 2020 18:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Back | FazBrowse Home | New Git URL