Repository navigation
Conversation
HexDecimal
left a comment
There was a problem hiding this comment.
Adding the affected runner should be enough for regression testing, but you'd have to show me that the workflow reproduces the error in the first place.
You'll also need to note what's been fixed in the changelog.
| matched = _RE_LIPO_UNKNOWN_FILE_STDERR.search(exc.stderr) | ||
| if not matched: |
There was a problem hiding this comment.
I worry about changing this to search. Maybe edit the expression to account for the match being towards the end.
| r"can't figure out the architecture type of: (?P<file>.+)\n$" | ||
| r"(" | ||
| r"fatal error: (?P<program>.+): " | ||
| r"can't figure out the architecture type of: (?P<file>.+)" |
There was a problem hiding this comment.
Why was \n$ removed from the end here?
There was a problem hiding this comment.
Given that the putative error here is "the error message was far too strict and an upgraded compiler breaks it", I felt like it made sense to loosen the matching strategy given that it seems unlikely to see this exact message even with a .search(). If you'd prefer I can definitely be stricter about it though.
There was a problem hiding this comment.
Given that the putative error here is "the error message was far too strict and an upgraded compiler breaks it"
This would be justified if the minor change was required for this fix, but in this case lipo's output was changed in a way that requires a whole new expression to parse. You should try your best not to tamper with the original expression unless you have a more well thought out refactor in mind.
There was a problem hiding this comment.
This would be justified if the minor change was required for this fix, but in this case
lipo's output was changed in a way that requires a whole new expression to parse. You should try your best not to tamper with the original expression unless you have a more well thought out refactor in mind.
Fair enough, I'll try to mirror the previous strictness.
Co-authored-by: Kyle Benesch <4b796c65+github@gmail.com>
There's a reproducer on the issue, does it not work for you? |
Reproducing the wheel isn't necessary since this issue applies to all wheels. More specifically, Delocate runs I'm inexperienced with MacOS so it's unclear to me if The new runners from GitHub are called |
I'm pretty sure it's the latter but sometimes I am surprised by details of this interaction. |
fixes #279
Pull Request Checklist