-
Notifications
You must be signed in to change notification settings - Fork 481
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
feat: add universal fix to allow python detection on DLLs #4538
feat: add universal fix to allow python detection on DLLs #4538
Conversation
@terriko Hi Terri! Wanted to reach out because this PR keeps failing some long tests. The only minor change here is modifying a regex string so I'm not sure what is causing these failures. Would you happen to have any insight as to what might be going wrong here? Really appreciate you taking a look, thank you! |
Yeah, something is definitely up with the long tests. Usually that kind of failure is transient so I was letting it sit for a couple of days, but if it's still happening this week then I'm going to need to switch how those are run. It's freezing halfway through the tests and I suspect since that one job (and only that one job) is being run on free-tier github actions at the moment, that may be the problem. |
Updating the branch to get the longtests to run with the new config. |
Wanted to follow up on this--seems like the new configuration didn't help with the failing longtest...is there something else we can try? Thanks in advance! |
I've filed an issue about what I think is going on with the longtests: #4602 I doubt I'm going to get it resolved today, but I'll send some questions out to the team that's handling our other runners and if they don't have an easy solution, I've scheduled some time on Friday to see if I find a different resolution. Sorry it's taking so long -- I got pulled into another project and the person who normally covers for me here is out on leave. |
No problem, thank you for all your help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think we've resolved the long test issues. I'm going to go ahead and update the branch which will re-run all the tests. I'll set this up to merge once they pass. Thank you again for your work on this and your incredible patience!
In a previous PR (#4023), I added a fix to allow for python3.11 detection on python DLLs, but this did not hold for python 3.12 detection. Here, I am modifying the version pattern to be more applicable across various python DLLs.
More detail: There's a string
tags/v3.xx.xx
that is followed by the stringversion_info
in 3.11, and the same stringtags/v3.xx.xx
exists in the 3.12 DLL but is followed by the stringunraisablehook
. So, updating the version pattern to be justr"tags/v([23]+\.[0-9]+\.[0-9]+)\r?\n"
instead ofr"tags/v([23]+\.[0-9]+\.[0-9]+)\r?\nversion_info"
as before. I tested this out, and successfully detected python version 3.12.6.