Ignore unparseable static libraries

[HexDecimal]

Pull Request Checklist

• Read and follow the CONTRIBUTING.md guide
• Mentioned relevant issues
• Append public facing changes to Changelog.md
• Ensure new features are covered by tests
• Ensure fixes are verified by tests

This branch was an attempt to remove macholib and write a better otool parser but I underestimated how difficult the output of otool was. I wasn't able to resolve cases where it'd decide to not return the architecture info in its output because it decided it was the current systems architecture. In the end I decided it was more reliable to suppress the error from macholib instead of trying to remove the library.

This suppresses ValueError: Unknown Mach-O header which fixes #229, fixes #227. I don't have a fat static binary to test this such as an archive (.a) compiled for both arm64 and x86_64.

While working on this I noticed the -m flag on otool and enabled it because delocate is always working with the whole file at once. This flag is documented as:

-m The object file names are not assumed to be in the archive(member) syntax, which allows file names containing parenthesis.

I also looked at the previous install dependencies, machomacomangler and bindepend, and removed them because there was no code using them. These looked like previous attempts at cross-platform handling of MacOS binary files.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.84%. Comparing base (e90c30c) to head (d247830).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
- Coverage   97.06%   96.84%   -0.22%     
==========================================
  Files          16       16              
  Lines        1361     1365       +4     
==========================================
+ Hits         1321     1322       +1     
- Misses         40       43       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HexDecimal]

Something to add to make_libs.sh to generate a fat arm64/x86_64 archive .a file would be nice for this. Any suggestions?

[matthew-brett]

This one looks fine to me. For my own benefit, the changes here are:

• Checking for, ignoring a known error frommacholib when parsing static libraries.
• Using the -m flag to otool, therefore allowing parentheses in input file names.
• Elevating some deprecation warnings to errors.

The only question I had was - is ignoring the static libraries the right thing? I guess it is, in that they cannot by definition be necessary for execution.

[HexDecimal]

Elevating some deprecation warnings to errors.

I don't think I've promoted any deprecation warnings to errors. Any deleted warning code was moved to a @deprecated decorator. This allows type linters to detect calls to deprecated functions.

The only question I had was - is ignoring the static libraries the right thing? I guess it is, in that they cannot by definition be necessary for execution.

This still checks static libraries. It's just that macholib does not support fat static libraries at the moment, so only single architecture static libraries will be analyzed. A version of macholib which does support them will automatically work as it won't throw an error anymore.

[HexDecimal]

There isn't a perfect solution to static libraries. Whether the static library is possibly loaded as part of the package logic or is solely used to create but not run other tools depends on the specific package.

It should be a simple change if we want to skip over static libraries during this minimum version analyses. It can even be toggled with a flag to delocate. Maybe count static libs by default but allow using a flag to ignore them.

[matthew-brett]

I don't think I've promoted any deprecation warnings to errors. Any deleted warning code was moved to a @deprecated decorator. This allows type linters to detect calls to deprecated functions.

Ah - yes - sorry - thanks for the clarification.

[matthew-brett]

It should be a simple change if we want to skip over static libraries during this minimum version analyses. It can even be toggled with a flag to delocate. Maybe count static libs by default but allow using a flag to ignore them.

That sounds good - do you think the function should error when it can't detect minimum version from static libs - as it does now - unless the ignore flag is set?

[HexDecimal]

That sounds good - do you think the function should error when it can't detect minimum version from static libs - as it does now - unless the ignore flag is set?

I don't have a preference on that, but the error is that macholib is unable to parse the file in the first place, so a failed parse doesn't mean the file was actually a fat static lib and not some other file type.

There's not much that can be done other than to fix the issue upstream: ronaldoussoren/macholib#49

[HexDecimal]

I added tests to verify ignoring static library info but it turns out these new tests demonstrate that static libraries do not have the LC_BUILD_VERSION or LC_VERSION_MIN_MACOSX commands in the first place. This means they don't have the minimum version info that's being checked for and there's no reason to ignore them specifically. My previous inference on static libraries having these attributes was accidental misinformation on my part. Dylib, shared objects, and object files have minimum version information.

Keeping this in mind, there's no difference between a static library and any non-library file as far as _get_macos_min_version is concerned. I no longer feel a need to test using a real fat static archive file. A test showing that current static libraries return no minimum version information is good enough, I think.

I've added # pragma: no cover to the unexpected exception line since there's no practical way to cover it.

[matthew-brett]

Ok - thanks for persisting. Is this one ready to go in now? You have an outstanding "Ensure new features are covered by tests" checkbox - did you mean (above) that you don't think you can practically do that?

[HexDecimal]

Ok - thanks for persisting. Is this one ready to go in now? You have an outstanding "Ensure new features are covered by tests" checkbox - did you mean (above) that you don't think you can practically do that?

I was considering the -m flag a feature that I wasn't testing for. I could probably figure out a way to write one more test for it, for completeness.

[HexDecimal]

Turns out I did have tests for -m, but I've ended up adding more. I've also verified just now that these tests do fail if the -m flag is removed.

[HexDecimal]

I also attempted to cover the _line0_says_object branches, but I couldn't figure out which files which would fail _line0_says_object while also passing _is_macho_file. I'm unsure what _line0_says_object is checking for.

[matthew-brett]

Looks good, thanks.