nghttp2: split libraries into separate libnghttp2 formula by Bo98 · Pull Request #86291 · Homebrew/homebrew-core

Bo98 · 2021-10-01T23:44:13Z

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?
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>?

This is a bit hacky, but it does work.

This reduces the dependency tree of curl.

carlocab · 2021-10-02T00:08:07Z

Formula/libnghttp2.rb

How much does building only the libraries speed up build time? I worry this adds some extra maintenance work without really all that much benefit.

In terms of the actual build of nghttp2 itself: probably not much.

This is purely with the idea that we can kill 3 dependencies from the curl dep tree so it might speed up that because you have to install less. And there was a couple of nghttp2 dependencies which I could not find plain HTTP mirrors for.

One benefit this might have is relocatable bottles, which will definitely improve install time. (Double check CI output in case I'm wrong.)

Yeah it is:

==> Bottling libnghttp2--1.45.1.arm64_big_sur.bottle.tar.gz... /opt/homebrew/opt/gnu-tar/bin/gtar --create --numeric-owner --format pax --owner 0 --group 0 --sort name --pax-option globexthdr.name=/GlobalHead.%n,exthdr.name=%d/PaxHeaders/%f,delete=atime,delete=ctime --file /Users/brew/actions-runner-x86_64/_work/homebrew-core/homebrew-core/bottles/libnghttp2--1.45.1.arm64_big_sur.bottle.tar libnghttp2/1.45.1 ./libnghttp2--1.45.1.arm64_big_sur.bottle.tar.gz bottle do sha256 cellar: :any, arm64_big_sur: "b6a81d7be284f3619e6d36551143152dc31a69e01e060e659397f5228f71fe89" end Writing libnghttp2--1.45.1.arm64_big_sur.bottle.json

==> Bottling nghttp2--1.45.1_1.arm64_big_sur.bottle.tar.gz... /opt/homebrew/opt/gnu-tar/bin/gtar --create --numeric-owner --format pax --owner 0 --group 0 --sort name --pax-option globexthdr.name=/GlobalHead.%n,exthdr.name=%d/PaxHeaders/%f,delete=atime,delete=ctime --file /Users/brew/actions-runner-x86_64/_work/homebrew-core/homebrew-core/bottles/nghttp2--1.45.1_1.arm64_big_sur.bottle.tar nghttp2/1.45.1_1 /opt/homebrew/Cellar/nghttp2/1.45.1_1/bin/nghttpx --> match '/opt/homebrew/Cellar/nghttp2/1.45.1_1/share/nghttp2/fetch-ocsp-response' at offset 0x8d601 Warning: String '/opt/homebrew/Cellar' still exists in these files: ./nghttp2--1.45.1_1.arm64_big_sur.bottle.tar.gz bottle do sha256 arm64_big_sur: "c3465c3d63b3dcd3c826db8fad58c6eb9c75dbb5e0f4c82ea6196d36f7217685" end Writing nghttp2--1.45.1_1.arm64_big_sur.bottle.json

Bo98 · 2021-10-02T03:38:24Z

@Homebrew/core Thoughts on the approach? This relates to trimming the dependency tree for curl, which soon everyone on macOS <= 10.15.5 will be forced to install.

Alternative is to just keep the dep tree as it is and find mirrors for a few more formulae, but I haven't managed to do this so far.

FreeBSD does a very similar approach to what I'm doing here. Arch Linux seems like it just does a full install and then deletes the libs (but that only works if the two packages install in the same place - which it will be in the keg-less world over there).

Bo98 · 2021-10-03T00:21:33Z

Ah, a pretty bad chicken & egg issue in Mojave CI.

carlocab · 2021-10-03T00:32:47Z

I suggest dropping all changes except for the ones dealing with nghttp2. You can create symlinks to libnghttp2* in nghttp2's lib directory for now so that dependents can find them.

The order in which test-bot does downloads and builds might still be a problem though...

Bo98 · 2021-10-03T00:34:37Z

I feel like it's always going to be an issue when we update any part of the dep tree.

Even when we get past the nghttp2 stuff, we face issues because we revision bumped curl:

 ==> Downloading https://ghcr.io/v2/homebrew/core/curl/manifests/7.79.1_1
curl: (22) The requested URL returned error: 404 
Error: curl: Failed to download resource "curl_bottle_manifest"
Download failed: https://ghcr.io/v2/homebrew/core/curl/manifests/7.79.1_1

Bo98 · 2021-10-03T00:36:27Z

And then once we've created the curl bottle, we uninstall the dependencies so it's completely broken from then on:

==> FAILED
/usr/local/Homebrew/Library/Homebrew/brew.sh: line 358: 39384 Abort trap: 6           "${HOMEBREW_PREFIX}/opt/curl/bin/curl" --version >&/dev/null

We had this issue with Git and the solution was to enforce /usr/bin/git usage.

carlocab · 2021-10-03T00:43:43Z

Yea, this clearly needs a more comprehensive fix. I'm just thinking splitting up nghttp2 here, and then rev-bumping curl in a separate PR (which only changes curl) will help getting this shipped to users sooner rather than later. We can figure out a proper fix after that.

Bo98 · 2021-10-03T00:46:47Z

This is a bit of a major shift in the direction we were going to take but it's an idea I just thought of that'll work better in CI and be much quicker to install for users:

How about we don't set HOMEBREW_FORCE_BREWED_CURL. We instead only install ca-certificates and then set SSL_CERT_FILE to be HOMEBREW_PREFIX/etc/ca-certificates/cert.pem. System curl is still usable in this scenario because in ca-certificates we filter out expired certificates.

carlocab · 2021-10-03T00:53:26Z

Sounds much nicer, yea.

Formula/node.rb

carlocab · 2021-10-04T03:58:36Z

Formula/nghttp2.rb

Why don't we want audit_result = true here?

I'm doing a glob over all Makefiles and some won't have this.

That seems worth documenting in a comment.

Ok. Don't really want to start another build unless I need to but I can do it on merge.

Yup, no need to restart the build for it.

The pattern I've seen used for this (that I like) is doing if s.include? here

I'm not entirely sure what extra safety that adds?

No safety but feels more self-documenting IMO.

Yeh, it'd need some work but may be worth doing eventually. Also worth thinking about this being possible for e.g. using a formula's url/sha in a resource.

carlocab · 2021-10-04T04:03:37Z

The ARM audit failure is annoying, especially since it prevents dependent tests.

Bo98 · 2021-10-04T04:05:40Z

Oh I didn't even realise it bailed out like that.

carlocab · 2021-10-04T04:07:28Z

Yea, I'm not a fan of this behaviour: #82220

Bo98 · 2021-10-04T04:07:43Z

Hmm there's a chicken egg problem here so I might need to do this after shipping Homebrew/brew#12167, which is annoying. The test-bot workaround equally broke things.

Bo98 · 2021-10-04T04:31:34Z

Ok pushed the curl mirror separately: 4ee61bd, since that was an important blocker.

I will remove the mirror from libnghttp2 that the audit is complaining about and will re-add it on merge (it's a new formula only audit).

Bo98 · 2021-10-04T04:36:26Z

Linux failed because... it's not using the latest brew for some reason:

Using Homebrew/brew 3.2.14-51-g1909d89d0 (Merge pull request #12165 from Bo98/http-mirror)

Bo98 · 2021-10-04T04:45:21Z

It's 5.45am so I'm done for now. I hoped to have gotten this finished before I went to bed but this build's going to take a few hours, particularly since I didn't even realise dependents were skipped.

If this works, and no one has any objections to the approach taken here, the plan is:

Merge this.
Merge resource_auditor: restore curl HTTP mirror auditing brew#12166.
Test on 10.10 VM.
Tag 3.2.15.

Rylan12 · 2021-10-04T04:47:13Z

Thanks for working so hard on this, @Bo98! Sleep well—you deserve it 😅

Bo98 · 2021-10-05T03:35:54Z

I have absolutely no idea why Linux has failed - it has built all bottles successfully. It's now unfortunately going to skip dependent testing...

Bo98 · 2021-10-05T03:54:54Z

I'm tempted to still merge this when it's done and then maybe open another PR to try get the dependent testing on Linux working.

carlocab · 2021-10-05T03:57:14Z

I'm tempted to still merge this when it's done and then maybe open another PR to try get the dependent testing on Linux working.

Works for me; but ping me when you do, as I'll need to rebase #85898 (also rev-bumps julia).

carlocab · 2021-10-05T04:01:02Z

I have absolutely no idea why Linux has failed - it has built all bottles successfully. It's now unfortunately going to skip dependent testing...

My guess is some missing conflicts_withs. There are a bunch of those on Linux.

Edit: Ah, wait, here's something:

==> Downloading https://curl.se/ca/cacert-2020-01-01.pem
/home/linuxbrew/.linuxbrew/bin/curl: error while loading shared libraries: libssh2.so.1: cannot open shared object file: No such file or directory
Trying a mirror...
==> Downloading https://gist.githubusercontent.com/dawidd6/16d94180a019f31fd31bc679365387bc/raw/ef02c78b9d6427585d756528964d18a2b9e318f7/cacert-2020-01-01.pem
/home/linuxbrew/.linuxbrew/bin/curl: error while loading shared libraries: libssh2.so.1: cannot open shared object file: No such file or directory
Warning: The post-install step did not complete successfully
You can try again using:
  brew postinstall gnutls

Bo98 · 2021-10-05T04:17:04Z

Wow that's hard to notice - nice find!

This is a resource("cacert").fetch in the gnutls postinstall. This line will disappear when we update gnutls to use ca-certificates.

The reason HOMEBREW_CURL_PATH isn't taking effect here is because we unset HOMEBREW_DEVELOPER here:

https://github.com/Homebrew/homebrew-test-bot/blob/e86c0e65fcb51edb5c85247fe361e3d166b8a7f6/lib/tests/formulae.rb#L305

but HOMEBREW_CURL_PATH requires HOMEBREW_DEVELOPER.

I think we should be removing the HOMEBREW_DEVELOPER restriction on HOMEBREW_CURL_PATH at some point. It's hacked around in Homebrew/install and is also a reason why some people disable env filtering.

carlocab · 2021-10-05T04:54:24Z

We could maybe just gate HOMEBREW_CURL_PATH with HOMEBREW_ALLOW_UNTRUSTED_CURL to be able to use it without HOMEBREW_DEVELOPER, and probably communicate better why we're gating HOMEBREW_CURL_PATH.

carlocab · 2021-10-05T11:52:03Z

Mojave failure from zero-install:

- BAD: Error fetching key info: Error downloading 'https://keylookup.0install.net/key/AC9B973549D819AE22BCD08D22EA111A7E4242A4': SSL certificate problem: certificate has expired

Bo98 · 2021-10-05T11:56:25Z

That'll likely be pre-existing from the general Let's Encrypt situation but worth noting as something to look at fixing.

carlocab · 2021-10-05T11:58:05Z

No Linux bottle. Probably actually needs (new-ish?) curl but doesn't have uses_from_macos.

Bo98 · 2021-10-05T12:08:40Z

Yeah probably a uses_from_macos "curl", since: :catalina.

BrewTestBot · 2021-10-05T12:20:39Z

@Bo98 has triggered a merge.

carlocab · 2021-10-05T13:06:08Z

This isn't great:

==> Installing dependencies for curl: libnghttp2
==> Installing curl dependency: libnghttp2
==> Pouring libnghttp2--1.45.1.big_sur.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink include/nghttp2/nghttp2.h
Target /usr/local/include/nghttp2/nghttp2.h
is a symlink belonging to nghttp2. You can unlink it:
  brew unlink nghttp2

To force the link and overwrite all conflicting files:
  brew link --overwrite libnghttp2

To list all files that would be deleted:
  brew link --overwrite --dry-run libnghttp2

Possible conflicting files are:
/usr/local/include/nghttp2/nghttp2.h -> /usr/local/Cellar/nghttp2/1.45.1/include/nghttp2/nghttp2.h
/usr/local/include/nghttp2/nghttp2ver.h -> /usr/local/Cellar/nghttp2/1.45.1/include/nghttp2/nghttp2ver.h
/usr/local/lib/libnghttp2.14.dylib -> /usr/local/Cellar/nghttp2/1.45.1/lib/libnghttp2.14.dylib
/usr/local/lib/libnghttp2.a -> /usr/local/Cellar/nghttp2/1.45.1/lib/libnghttp2.a
/usr/local/lib/libnghttp2.dylib -> /usr/local/Cellar/nghttp2/1.45.1/lib/libnghttp2.dylib
/usr/local/lib/pkgconfig/libnghttp2.pc -> /usr/local/Cellar/nghttp2/1.45.1/lib/pkgconfig/libnghttp2.pc
==> Summary
🍺  /usr/local/Cellar/libnghttp2/1.45.1: 13 files, 659.9KB

The problem is that brew installed libnghttp2 before upgrading nghttp2. I wonder if there's a way for us to enforce upgrade order...

Bo98 · 2021-10-05T13:07:37Z

That's annoying because I don't know a quick fix for that.

MikeMcQuaid · 2021-10-05T13:08:11Z

link_overwrite?

Bo98 · 2021-10-05T13:08:40Z

Ah yes, good idea. Let's do that.

Bo98 added CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-linux-self-hosted Build on Linux self-hosted runner CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Oct 1, 2021

carlocab reviewed Oct 2, 2021

View reviewed changes

Bo98 force-pushed the libnghttp2 branch from ee085b3 to 684b045 Compare October 2, 2021 00:32

carlocab added the maintainer feedback Additional maintainers' opinions may be needed label Oct 2, 2021

Bo98 mentioned this pull request Oct 3, 2021

Install and use Homebrew's ca-certificates on macOS <= 10.15.5 Homebrew/brew#12167

Merged

7 tasks

Bo98 removed the CI-linux-self-hosted Build on Linux self-hosted runner label Oct 3, 2021

carlocab reviewed Oct 3, 2021

View reviewed changes

Formula/node.rb Outdated Show resolved Hide resolved

Bo98 added new formula PR adds a new formula to Homebrew/homebrew-core CI-linux-self-hosted Build on Linux self-hosted runner labels Oct 3, 2021

Bo98 force-pushed the libnghttp2 branch from 684b045 to 89b152b Compare October 4, 2021 02:07

carlocab reviewed Oct 4, 2021

View reviewed changes

Bo98 force-pushed the libnghttp2 branch from 89b152b to 1113f19 Compare October 4, 2021 04:40

Bo98 added 8 commits October 5, 2021 00:06

dnsperf: use libnghttp2

1e61a6e

httpd: use libnghttp2

1c00dc0

knot: use libnghttp2

a8c0839

julia: use libnghttp2

b7bde16

node: use libnghttp2 and our CA store

9be7ea4

unbound: use libnghttp2

6d58a87

wireshark: use libnghttp2

99189b3

knot-resolver: use libnghttp2

146dc6c

Bo98 force-pushed the libnghttp2 branch from 49d1b28 to 146dc6c Compare October 4, 2021 23:07

BrewTestBot closed this in 673a815 Oct 5, 2021

This was referenced Oct 5, 2021

libnghttp2: fix conflict error on upgrading #86493

Merged

MacOS system curl broken after letsencrypt CA cert expiry Homebrew/brew#12161

Closed

node(@*): fix test #86090

Closed

github-actions bot added the outdated PR was locked due to age label Nov 5, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2021

Uh oh!

Conversation

Bo98 commented Oct 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bo98 commented Oct 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bo98 commented Oct 3, 2021

Uh oh!

carlocab commented Oct 3, 2021

Uh oh!

Bo98 commented Oct 3, 2021

Uh oh!

Bo98 commented Oct 3, 2021

Uh oh!

carlocab commented Oct 3, 2021

Uh oh!

Bo98 commented Oct 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlocab commented Oct 3, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlocab commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bo98 commented Oct 4, 2021

Uh oh!

carlocab commented Oct 4, 2021

Uh oh!

Bo98 commented Oct 4, 2021

Uh oh!

Bo98 commented Oct 4, 2021

Uh oh!

Bo98 commented Oct 4, 2021

Uh oh!

Bo98 commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rylan12 commented Oct 4, 2021

Uh oh!

Bo98 commented Oct 5, 2021

Uh oh!

Bo98 commented Oct 5, 2021

Uh oh!

carlocab commented Oct 5, 2021

Uh oh!

carlocab commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bo98 commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlocab commented Oct 5, 2021

Uh oh!

carlocab commented Oct 5, 2021

Bo98 commented Oct 2, 2021 •

edited

Loading

Bo98 commented Oct 3, 2021 •

edited

Loading

carlocab commented Oct 4, 2021 •

edited

Loading

Bo98 commented Oct 4, 2021 •

edited

Loading

carlocab commented Oct 5, 2021 •

edited

Loading

Bo98 commented Oct 5, 2021 •

edited

Loading