Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1055 +/- ##
=======================================
Coverage 92.31% 92.31%
=======================================
Files 103 103
Lines 13363 13366 +3
Branches 483 483
=======================================
+ Hits 12336 12339 +3
Misses 1027 1027
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
blin
left a comment
blin
left a comment
There was a problem hiding this comment.
It would be useful to have the context from #1048 in the commit message, specifically "record the version of Python that the capture file was generated with, not the version of Python that the Memray module was compiled with".
LGTM, but please see one nit before merging.
| return static_cast<int>(Py_Version); | ||
| #else | ||
| assert(PyGILState_Check()); | ||
| PyObject* hexversion = PySys_GetObject("hexversion"); // Borrowed reference |
There was a problem hiding this comment.
nit: sys.hexversion is writeable, there is an edge case where overwritten sys.hexversion causes the wrong branch in parseLinetable to be taken. I'm guessing that this is not a practical concern, but would defer to you on whether this is something worth guarding against / worth mentioning in the commit message as not worth worrying about.
There was a problem hiding this comment.
No, not worth worrying about, I think. We've hardened ourselves against malformed line tables, and a well formed line table that's being parsed by the wrong version's parse function would just get treated as malformed. I think that's fine, especially considering this will go away when we drop support for 3.10 in a year or so.
There was a problem hiding this comment.
Actually, you know what? Let's just keep returning PY_VERSION_HEX for 3.9 and 3.10. It's less useful for us, but it can't be tampered with, and we won't be supporting those versions much longer anyway.
For Python 3.11 and newer, record the version of Python that the capture file was generated with rather than the version of Python that the Memray module was compiled with. The runtime version is more useful. For Python 3.9 and 3.10, continue recording the version of Python that Memray was built against, as these versions don't have a convenient way to get the runtime version in the format we need. `Py_Version` was introduced in 3.11, and while `sys.hexversion` exists for earlier versions it's also able to be overwritten by the user, so we choose not to trust it. Also, accessing it requires holding the GIL. Signed-off-by: Matt Wozniski <godlygeek@gmail.com>
0774f4f to
370e204
Compare
|
Give this another review, please, @blin. |
This branch has not been deployed
3 participants
Previously we were capturing the compile time version instead, which is less useful.