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

hashlink 1.12 #113509

Closed
wants to merge 2 commits into from
Closed

hashlink 1.12 #113509

wants to merge 2 commits into from

Conversation

tobil4sk
Copy link
Contributor

@tobil4sk tobil4sk commented Oct 19, 2022

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
    @chenrui333 suggested opening a new one: hashlink 1.12 #100517 (comment)
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Due to a make issue, we need to fix the PCRE_INCLUDE variable name. On Linux, the rpath as it is set automatically now for the main executables, however, we still need to modify the makefile to set it for the .hdll files. On Mac, we need to still set the rpath so that the hdll libraries can be loaded correctly.

@BrewTestBot BrewTestBot added the no ARM bottle Formula has no ARM bottle label Oct 19, 2022
@tobil4sk tobil4sk mentioned this pull request Oct 19, 2022
chenrui333
chenrui333 previously approved these changes Oct 19, 2022
@chenrui333 chenrui333 added ready to merge PR can be merged once CI is green automerge-skip `brew pr-automerge` will skip this pull request labels Oct 19, 2022
@chenrui333
Copy link
Member

Thanks @tobil4sk!!

@chenrui333
Copy link
Member

Go for the merge?

@iMichka
Copy link
Member

iMichka commented Nov 3, 2022

Given there was already some rpath hacking done in the formula and it got just fixed/improved, let's ship this. But ideally we would like to get some of these upstreamed so that we do not need to carry patches around.

iMichka
iMichka previously approved these changes Nov 3, 2022
@tobil4sk
Copy link
Contributor Author

tobil4sk commented Nov 3, 2022

Perhaps there would be a better way of handling the load paths for macOS, to avoid the rpath hacks? The hack is needed there only because .hdlls fail to load properly from libexec/lib. It's odd however that this is only an issue for the .hdlls and not the standard hashlink dylib. Perhaps the formula should somehow mark the hdlls as dynamic libraries so homebrew can do whatever it does that gets the dylib working properly? Or perhaps this should be handled in some other way?

For the linux patch, I'll try to do a PR to merge it upstream, however, it still won't come until the next hashlink release which won't be for a while.

@iMichka
Copy link
Member

iMichka commented Nov 3, 2022

For the linux patch, I'll try to do a PR to merge it upstream, however, it still won't come until the next hashlink release which won't be for a while.

That's fine, the important part it comes in a update in the next months/years, and not in 10 years if no-one submits a patch. We know that release timelines can be quite long, so if we can anticipate things that's always good.

@tobil4sk
Copy link
Contributor Author

tobil4sk commented Nov 3, 2022

I've opened a PR for the linux patch: HaxeFoundation/hashlink#571

I'm not sure what else to do about the linking on macOS, as I don't know much about dynamic linking practices for homebrew on macOS.

@fxcoudert fxcoudert dismissed stale reviews from iMichka and chenrui333 via dc136bf November 14, 2022 13:37
@BrewTestBot BrewTestBot removed the automerge-skip `brew pr-automerge` will skip this pull request label Nov 14, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Dec 6, 2022
@chenrui333 chenrui333 removed the stale No recent activity label Dec 10, 2022
@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@github-actions github-actions bot added the outdated PR was locked due to age label Jan 17, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2023
@tobil4sk tobil4sk deleted the bump-hashlink-1.12 branch January 31, 2023 01:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no ARM bottle Formula has no ARM bottle outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants