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

xxhash: Fix 10.7 Lion clang segfault #19899

Merged
merged 2 commits into from
Aug 18, 2023
Merged

Conversation

thestr4ng3r
Copy link
Contributor

Description

Fixes: https://trac.macports.org/ticket/67963
May supersede #19897

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.7.5 11G63 x86_64
Xcode 4.6.3 4H1503

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test? (There are none)
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@macportsbot
Copy link

Notifying maintainers:
@Schamschula for port xxhash.

@thestr4ng3r
Copy link
Contributor Author

Sent upstream too: Cyan4973/xxHash#878

Copy link
Contributor

@chrstphrchvz chrstphrchvz left a comment

Choose a reason for hiding this comment

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

Normally a build fix does not require a revision increase, but it may be necessary in this case since it affects an installed file.

Is it really not preferable to apply this patch regardless of macOS/compiler version? It looks like it can be applied unconditionally, and I would think doing so is a good idea, in part because those maintaining the port on more recent macOS would notice if it breaks e.g. during updates.

@thestr4ng3r
Copy link
Contributor Author

Yes, I think it should be possible to do unconditionally. I agree that less control flow in the portfile helps maintenance.

Should we maybe update the revision only for the xxhashlib subport since that is the one that installs the header? The xxhash one should result in identical binaries if it compiled before.

@chrstphrchvz
Copy link
Contributor

Should we maybe update the revision only for the xxhashlib subport since that is the one that installs the header? The xxhash one should result in identical binaries if it compiled before.

I agree with this.

@mascguy mascguy self-assigned this Aug 16, 2023
Copy link
Member

@mascguy mascguy left a comment

Choose a reason for hiding this comment

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

I was able to reproduce the crash on my 10.7 VM, and can confirm that this patch fixes the issue. Nice work!

@Schamschula Marius, this is blocking other ports from building on 10.7, so it would be great to merge this ASAP. Can you take a look when you get a chance? (The changes are trivial, so it's a 2-minute review at most.)

@mascguy
Copy link
Member

mascguy commented Aug 18, 2023

Maintainer timeout, merging.

@mascguy mascguy merged commit ab56482 into macports:master Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer: open Affects an openmaintainer port type: bugfix
Development

Successfully merging this pull request may close these issues.

5 participants