fix(goblin): disable PE import table parser#964
Conversation
|
Just FYI, I proposed an alternative PR upstream that fixes the underlying issue instead of adding an import parser flag: m4b/goblin#527 |
supervacuus
force-pushed
the
fix/disable_goblin_import_parser
branch
from
681b909 to
830af82
Compare
|
The option was merged upstream (m4b/goblin#522) and released with 0.10.6.
|
Co-authored-by: David Herberth <[email protected]>
|
Before I merge this, to verify, since I created this PR initially from a different premise:
However, the tests (+snapshots and fixtures) I introduced in #960 are now missing, right? That should still be done, right? Otherwise, a sensible summary? Regarding release plans and potential bumps, are we good? |
Good catch, yes, the tests are gone and it would be really useful to have them back!
Yep, that looks all good to me. Let's wait for @loewenheim as well, he has a lot more context. |
|
Everything seems in order now, thanks for picking this up again @supervacuus! |
### Description Extracted from #3237 Some tests currently fail, because `symbolic` updated its `zip` dependency (`2.4.2` to `7.2.0`) since the last bump. This changed the encoding and invalidated a bunch of test assertions. A fix proposal for the failing tests is #3239. This change was triggered by bumping `symbolic` to `12.17.3` to include getsentry/symbolic#960. The new permissive PE parser fixes in getsentry/symbolic#960 exposed performance edge cases for some of the debug companions during PE import-table parsing, the result of which neither `symbolic` nor `sentry-cli` really needs. The PR addresses the issue in the following way: I introduced a new `goblin` option that excludes the `PE` import table from parsing. Of course, this requires further upstream PRs (`cargo` points `goblin` and `symbolic` to dev branches) before we can merge this, but I wanted to check whether you are generally in agreement with these changes before going upstream. Upstream changes required: - m4b/goblin#522 - getsentry/symbolic#964 Ideally, this PR gets merged after #3238 and #3239, because both solve issues this PR is affected by. If we drop or defer #3239, we need to update all snapshots and magic digests here first. ### Issues At least partially fixes getsentry/sentry#104738 ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
Bump `symbolic` to [13.1.1 ](https://github.com/getsentry/symbolic/releases/tag/13.1.1)to align with debug companion support in `sentry-cli` [3.5.0](https://github.com/getsentry/sentry-cli/releases/tag/3.5.0) Part of the fixes for: #104738 Related changes: getsentry/sentry-cli#3240 getsentry/symbolic#964 cc: @szokeasaurusrex, @Dav1dde, @loewenheim
3 participants
This is a follow-up to #960 and depends on an upstream change in
goblinthat has not yet been accepted (m4b/goblin#522).It makes the integration of PE+DWARF debug companions into the DIF pipeline practical (see getsentry/sentry-cli#3240)