-
Notifications
You must be signed in to change notification settings - Fork 45
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
Fix sros2 tests on Windows Debug. #317
Conversation
Thanks 🙏 Ideally I'd like to keep separate the behavioral fix and API/signature change to streamline fix back porting to older distros. Finally is it possible to trigger a windows debug job once the PR splitted to make sure the CI is happy about it ? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## rolling #317 +/- ##
===========================================
+ Coverage 88.92% 88.94% +0.01%
===========================================
Files 24 24
Lines 614 615 +1
Branches 64 64
===========================================
+ Hits 546 547 +1
Misses 50 50
Partials 18 18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Actually, to be perfectly frank we could just leave the order as-is and remove all of the default arguments. This function is always called with all of its arguments. So I think I'll open a separate PR to do that. |
From what I can tell, it looks like lxml has a bug where it doesn't properly track references to objects via the find() method. This manifests on Windows debug as a crash *after* we have stopped using the object, but I believe that by that point the underlying memory has already been freed. Windows Debug in particular is sensitive to this. Fix it by doing a deepcopy of the object returned from the find(). This code isn't performance sensitive, so it shouldn't be a big deal to do it here, and it fixes the bug in my testing. Signed-off-by: Chris Lalancette <[email protected]>
34ac294
to
4f136df
Compare
And see #318 for the rest of the fixes. |
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.
lgtm with green CI, thanks for the fix.
Cheers! |
@Mergifyio backport jazzy |
✅ Backports have been created
|
From what I can tell, it looks like lxml has a bug where it doesn't properly track references to objects via the find() method. This manifests on Windows debug as a crash *after* we have stopped using the object, but I believe that by that point the underlying memory has already been freed. Windows Debug in particular is sensitive to this. Fix it by doing a deepcopy of the object returned from the find(). This code isn't performance sensitive, so it shouldn't be a big deal to do it here, and it fixes the bug in my testing. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit bbccb6a)
From what I can tell, it looks like lxml has a bug where it doesn't properly track references to objects via the find() method. This manifests on Windows debug as a crash *after* we have stopped using the object, but I believe that by that point the underlying memory has already been freed. Windows Debug in particular is sensitive to this. Fix it by doing a deepcopy of the object returned from the find(). This code isn't performance sensitive, so it shouldn't be a big deal to do it here, and it fixes the bug in my testing. Signed-off-by: Chris Lalancette <[email protected]> (cherry picked from commit bbccb6a) Co-authored-by: Chris Lalancette <[email protected]>
From what I can tell, it looks like lxml has a bug where it doesn't properly track references to objects via the find() method. This manifests on Windows debug as a crash after we have stopped using the object, but I believe that by that point the underlying memory has already been freed. Windows Debug in particular is sensitive to this.
Fix it by doing a deepcopy of the object returned from the find(). This code isn't performance sensitive, so it shouldn't be a big deal to do it here, and it fixes the bug in my testing.