Skip to content
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

more mypy v1.1.1 fixes #1423

Merged
merged 15 commits into from
Apr 3, 2023
Merged

more mypy v1.1.1 fixes #1423

merged 15 commits into from
Apr 3, 2023

Conversation

williballenthin
Copy link
Collaborator

@williballenthin williballenthin commented Apr 3, 2023

fixes:

  • mypy v1.1.1 for newly merged code
  • more thorough deserialization of result document, as identified by mypy
  • elf typing
  • elf test case failures

Checklist

  • No CHANGELOG update needed
  • No new tests needed
  • No documentation update needed

@williballenthin

This comment was marked as resolved.

@williballenthin

This comment was marked as resolved.

@mr-tz mr-tz added this to the vNext milestone Apr 3, 2023
@williballenthin williballenthin changed the title Mypy 111 more mypy v1.1.1 fixes Apr 3, 2023
@williballenthin williballenthin marked this pull request as ready for review April 3, 2023 14:09
Comment on lines -777 to -788
SHT_SYMTAB = 0x2
SHT_STRTAB = 0x3
strtab_buf = symtab_buf = None

for shdr in elf.section_headers:
if shdr.type == SHT_STRTAB:
strtab_buf, strtab_sz= shdr.buf, shdr.size

elif shdr.type == SHT_SYMTAB:
symtab_buf, symtab_entsize, symtab_sz = shdr.buf, shdr.entsize, shdr.size

if None in (strtab_buf, symtab_buf):
Copy link
Collaborator Author

@williballenthin williballenthin Apr 3, 2023

Choose a reason for hiding this comment

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

@yelhamer FYI, ELF files may have mutiple strtabs, so we need to read the correct one using the sh_link field of the symtab section header field, rather than just taking the last one found. i learned this from here: https://stackoverflow.com/a/69888949/87207

i noticed the bug because the ELF sample bf7a9c8bdfa6d47e01ad2b056264acc3fd90cf43fe0ed8deec93ab46b47d76cb has extra string tables that didn't line up with what the symtab entries expected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note i've also done a little refactoring to make use of existing code to parse section headers and their associated data. no major changes to your logic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I missed using sh_link to get strtab's index, that's pretty nifty!

I also really like the refactoring you did, specifically, adding symtab() to the ELF section, I think that's much better than my original solution (having it inside the guess_os_from_symtab() function).

Thank you!

@williballenthin
Copy link
Collaborator Author

let's ensure tests pass, and if they do, @mr-tz should consider this for code review and merge, so we can get master green again.

Copy link
Collaborator

@mr-tz mr-tz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!!

@mr-tz
Copy link
Collaborator

mr-tz commented Apr 3, 2023

PyInstaller fail appears unrelated (GitHub issue?).

@williballenthin
Copy link
Collaborator Author

actions/runner-images#6002

the ubuntu-18.04 image has been deprecated

@williballenthin
Copy link
Collaborator Author

i think we were using the oldest ubuntu possible so that the resulting builds would work with the widest range of commonly installed dependencies. by updating to ubuntu 20.04, we may fail to run on other older linuxes (ubuntu 18.04, 18.10, etc.). lets try this out, and if we face pushback, we can find another old linux image.

@williballenthin williballenthin merged commit acd0020 into master Apr 3, 2023
@williballenthin williballenthin deleted the mypy-111 branch April 3, 2023 19:48
@mr-tz mr-tz mentioned this pull request Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants