homebrew-core icon indicating copy to clipboard operation
homebrew-core copied to clipboard

newnode-helper 2.1.4 (new formula)

Open theoden8 opened this issue 1 year ago • 5 comments

  • [x] Have you followed the guidelines for contributing?
  • [x] Have you ensured that your commits follow the commit style guide?
  • [x] Have you checked that there aren't other open pull requests for the same formula update/change?
  • [x] Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • [x] Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • [x] Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Newnode is an open-source decentralized Content Distribution Network (https://github.com/clostra/newnode).

This is a rewrite of #153960 with brew services in mind. Tested on intel OSx 14 Sonoma.

theoden8 avatar Feb 14 '24 13:02 theoden8

Thanks for contributing to Homebrew! :tada: It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

github-actions[bot] avatar Feb 14 '24 13:02 github-actions[bot]

This seems to be vendoring a bunch of homebrew formula, which violates: https://docs.brew.sh/Acceptable-Formulae#stuff-that-requires-vendored-versions-of-homebrew-formulae

  Entering 'libevent'
  Entering 'libsodium'

The codebase of newnode uses internal headers of libevent (e.g. in https://github.com/clostra/newnode/blob/master/bufferevent_utp.h), which are not provided in homebrew version. Additionally, our fork of libevent includes important fixes that would lead to errors in the code.

libsodium, however, I think I can try and make it depend on externally.

Would that be a reasonable compromise to resolve this issue?

theoden8 avatar Feb 14 '24 15:02 theoden8

@SMillerDev any update on this? I can try and look into fixing https://github.com/Homebrew/homebrew-core/pull/162684 (though, I can't promise it), and we're very interested in making the project (d2d vpn with dht) accessible for mac users.

theoden8 avatar Feb 21 '24 14:02 theoden8

Additionally, our fork of libevent includes important fixes that would lead to errors in the code.

Have they been submitted upstream? Then we can just patch the libevent formula

SMillerDev avatar Feb 21 '24 14:02 SMillerDev

The fork of libevent we're using has in part been submitted in a series of pull requests (see https://github.com/libevent/libevent/pulls/ghazel), and they might be resubmitted down the line. But this would take a considerable amount of time.

In the meantime, we use features like allow external access to evdns cache, as well as internal headers and ones resulting from compiling libevent (e.g. evconfig.h).

@SMillerDev what do you mean by patching libevent formula, and does it allow for this flexibility?

theoden8 avatar Feb 21 '24 16:02 theoden8

We've released the new version which should be more compatible with homebrew's policy:

  1. Upon closer inspection, mbedtls isn't needed, so it is no longer listed as a dependency (neither, the new nor the old version).
  2. We've updated libevent, which now have some of the functionality similar to that submitted upstream.
  3. Updated libsodium (we still need it as a controlled dependency for mobile apps).

Does this provide a way forward with this PR?

theoden8 avatar Feb 29 '24 12:02 theoden8

@SMillerDev are we any closer to completion? It's been over a month since this PR started, and several key concerns have been addressed.

theoden8 avatar Mar 12 '24 15:03 theoden8

Any update on this? What's missing?

theoden8 avatar Mar 19 '24 11:03 theoden8

Any update on this? @SMillerDev I believe most comments have been addressed. We do not use liblzma bug in our codebase.

theoden8 avatar Apr 09 '24 09:04 theoden8

Anything else that need be addressed? @SMillerDev

theoden8 avatar May 07 '24 17:05 theoden8

Any further updates on this? @SMillerDev

theoden8 avatar May 21 '24 16:05 theoden8

Please don't open PR's from organization forks. It prevents maintainers from pushing fixes, and also breaks our merge process.

p-linnane avatar May 22 '24 17:05 p-linnane

Please don't open PR's from organization forks. It prevents maintainers from pushing fixes, and also breaks our merge process.

@p-linnane sure, we can either allow access to avoid re-creating this PR or open it from a personal account. Sorry, I didn't realize there was an issue.

Thanks for the PR, but I don't think we can look past the vendor-ing of multiple pieces of software that are provided by Homebrew.

There are already plenty of existing formulae that do this. We're trying to fix them so they stop doing that, but adding another formula that does it just makes that job harder.

Apologies for the extended review process here. We still appreciate the work you put in. I've left suggestions that will help you improve the formula for when it's ready for inclusion into Homebrew/core.

If you wish, you can host this formula in your own tap until then. Here are some docs to help you get started:

https://docs.brew.sh/Taps https://docs.brew.sh/Interesting-Taps-and-Forks https://docs.brew.sh/How-to-Create-and-Maintain-a-Tap https://brew.sh/2020/11/18/homebrew-tap-with-bottles-uploaded-to-github-releases/

@carlocab if we excluded libsodium upstream, in a fork or in a branch, would that be more acceptable? I believe prior communication on that matter left an impression that two vendored formulae was a permissible compromise, and other issues had to be ironed out first. If that's the case, let's reopen it and we will find a way to reduce the load on homebrew maintainers.

theoden8 avatar May 24 '24 22:05 theoden8

@carlocab @SMillerDev should I move these questions somewhere else, e.g. discussions/issues? I understand the concern, but I don't know how to resolve it without some back-and-forth given that the same newnode repo is used for building ios and android packages too. We really want the package to be included but we don't want to spend your time and mental energy figuring out too many nuances.

theoden8 avatar Jun 04 '24 11:06 theoden8

Essentially we don't want to vendor anything. If the software was super popular (think Chrome or Nginx) we might lift that restriction, but since this tool is not I'd suggest upstreaming your fixes and then you can try again.

Or, as other maintainers pointed out, make a tap.

SMillerDev avatar Jun 04 '24 12:06 SMillerDev