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 #100517

Closed
wants to merge 1 commit into from
Closed

Conversation

chenrui333
Copy link
Member

Created with brew bump-formula-pr.

@BrewTestBot BrewTestBot added bump-formula-pr PR was created using `brew bump-formula-pr` no ARM bottle Formula has no ARM bottle labels May 1, 2022
@chenrui333 chenrui333 added the build failure CI fails while building the software label May 1, 2022
@cho-m
Copy link
Member

cho-m commented May 1, 2022

Maybe due to HaxeFoundation/hashlink@880b81d

Looks like upstream bundles pcre. The Makefile was refactored with variable PCRE_INCLUDE but the actual variable used is PCRE_FLAGS

@github-actions
Copy link
Contributor

github-actions bot commented May 4, 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. To keep this pull request open, add a help wanted or in progress label.

@github-actions github-actions bot added the stale No recent activity label May 4, 2022
@github-actions github-actions bot closed this May 5, 2022
@tobil4sk
Copy link
Contributor

tobil4sk commented May 8, 2022

I tried to create a PR for this update but brew bump-formula-pr redirected me here.

The make file issue is indeed as diagnosed above, so until the next version is released (which probably won't be for a while), the best solution would be to fix it manually. Also, a bunch of the other patches can now be removed because with 1.12, the rpath issues they were trying to address have been resolved. Just requires setting the prefix for both make commands.

def install
  inreplace "Makefile", /PCRE_INCLUDE =/, "PCRE_FLAGS ="
  system "make", "PREFIX=#{libexec}"
  system "make", "install", "PREFIX=#{libexec}"
  bin.install_symlink Dir[libexec/"bin/*"]
end

@chenrui333 chenrui333 reopened this May 15, 2022
@chenrui333 chenrui333 force-pushed the bump-hashlink-1.12 branch from bfc97a4 to 7794a04 Compare May 15, 2022 01:04
@chenrui333
Copy link
Member Author

@tobil4sk adopted your build update.

@chenrui333 chenrui333 added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label May 15, 2022
@github-actions github-actions bot removed the stale No recent activity label May 15, 2022
@tobil4sk
Copy link
Contributor

Thanks! After a conversation in #99237, it would seem that adding the rpath to the libraries is still necessary, so this should address that issue:

def install
  # For Linux, also set RPATH in LIBFLAGS, so that the linker will also add the RPATH to .hdll files.
  inreplace "Makefile", "LIBFLAGS =", "LIBFLAGS = -Wl,-rpath,${INSTALL_LIB_DIR}"
  inreplace "Makefile", /PCRE_INCLUDE =/, "PCRE_FLAGS ="
  system "make", "PREFIX=#{libexec}"
  system "make", "install", "PREFIX=#{libexec}"
  bin.install_symlink Dir[libexec/"bin/*"]
end

@tobil4sk
Copy link
Contributor

Hm, looking at the other errors, perhaps setting the rpath for the main executables is still necessary for now on macos.

@tobil4sk
Copy link
Contributor

So maybe something like this is needed until the makefile is fixed:

def install
  # For Linux, also set RPATH in LIBFLAGS, so that the linker will also add the RPATH to .hdll files.
  inreplace "Makefile", "LIBFLAGS =", "LIBFLAGS = -Wl,-rpath,${INSTALL_LIB_DIR}" if os.linux?
  inreplace "Makefile", /PCRE_INCLUDE =/, "PCRE_FLAGS ="

  if os.mac?
    # make file doesn't set rpath on mac yet
    system "make", "PREFIX=#{libexec}", "EXTRA_LFLAGS=-Wl,-rpath,#{libexec}/lib"
  elsif os.linux?
    system "make", "PREFIX=#{libexec}"
  end

  system "make", "install", "PREFIX=#{libexec}"
  bin.install_symlink Dir[libexec/"bin/*"]
end

@github-actions
Copy link
Contributor

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. To keep this pull request open, add a help wanted or in progress label.

@github-actions github-actions bot added the stale No recent activity label May 18, 2022
@github-actions github-actions bot closed this May 19, 2022
@chenrui333 chenrui333 reopened this Oct 19, 2022
@chenrui333
Copy link
Member Author

let's give it another run

@github-actions github-actions bot removed the stale No recent activity label Oct 19, 2022
Signed-off-by: Rui Chen <[email protected]>
@chenrui333
Copy link
Member Author

Also @tobil4sk, feel free to submit a new PR.

@tobil4sk
Copy link
Contributor

let's give it another run
Also @tobil4sk, feel free to submit a new PR.

Alright, thanks! I might do that just to check that the -rpath line is 100% necessary on macOS or whether we can just get rid of that if block.

@tobil4sk tobil4sk mentioned this pull request Oct 19, 2022
6 tasks
@tobil4sk
Copy link
Contributor

I've opened #113509.

@chenrui333 chenrui333 closed this Oct 19, 2022
@chenrui333
Copy link
Member Author

Thanks @tobil4sk!

@chenrui333 chenrui333 added the superseded PR was replaced by another PR label Oct 19, 2022
@github-actions github-actions bot added the outdated PR was locked due to age label Nov 19, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2022
@chenrui333 chenrui333 deleted the bump-hashlink-1.12 branch March 12, 2023 07:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build failure CI fails while building the software bump-formula-pr PR was created using `brew bump-formula-pr` CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. no ARM bottle Formula has no ARM bottle outdated PR was locked due to age superseded PR was replaced by another PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants