newnode 2.1.5 (new formula) #162683

theoden8 · 2024-02-14T13:43:28Z

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 HOMEBREW_NO_INSTALL_FROM_API=1 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 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.

Formula/n/newnode-helper.rb

github-actions · 2024-02-14T13:59:41Z

Thanks for contributing to Homebrew! 🎉 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.

Formula/n/newnode-helper.rb

SMillerDev

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'

theoden8 · 2024-02-14T15:20:50Z

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 · 2024-02-21T14:40:25Z

@SMillerDev any update on this? I can try and look into fixing #162684 (though, I can't promise it), and we're very interested in making the project (d2d vpn with dht) accessible for mac users.

SMillerDev · 2024-02-21T14:42:05Z

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

theoden8 · 2024-02-21T16:58:37Z

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?

p-linnane

Thanks for the PR. I have some concerns I need addressed before we attempt to move forward with this.

Formula/n/newnode-helper.rb

further inspection of https://github.com/clostra/newnode/blob/master/build.sh and subsequent build tests confirm that wget isnt needed on mac os

instead of assuming that the proxy is already run as a service, run it in a fork and kill at completion

linux builds currently rely on a hard-coded wget binary, which needs to change upstream to non-hardcoded binary

fix brew audit fail due to non-alphabetic ordering of dependencies

most of the dependencies specified so far are actually build-time only, and pkg-config was not specified at all

updated newnode-helper to 2.1.5, with updated dependencies and no dependency on mbedtls

theoden8 · 2024-02-29T12:56:30Z

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

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

Does this provide a way forward with this PR?

Formula/n/newnode-helper.rb

Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>

Formula/n/newnode.rb

Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>

theoden8 · 2024-04-09T09:03:32Z

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

SMillerDev · 2024-04-11T05:57:02Z

Formula/n/newnode.rb

+      begin
+        Timeout.timeout(5) do
+          Process.wait(pid)
+        end
+      rescue Timeout::Error
+        Process.kill("KILL", pid) # Forcefully kill if not terminated after timeout
+        Process.wait(pid)
+      end


Why would we expect it not to be killed by the kill above?

Removed the second kill

Formula/n/newnode.rb

theoden8 · 2024-05-07T17:35:57Z

Anything else that need be addressed? @SMillerDev

theoden8 · 2024-05-21T16:34:18Z

Any further updates on this? @SMillerDev

p-linnane · 2024-05-22T17:27:07Z

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

carlocab

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 · 2024-05-22T17:41:42Z

Formula/n/newnode.rb

+  on_macos do
+    depends_on xcode: ["9.3", :build]
+  end


You don't need the on_macos block here.

carlocab · 2024-05-22T17:43:30Z

Formula/n/newnode.rb

+    tag:      "2.1.5",
+    revision: "18a3f713267b2a08e34cbe04253df891889997a2"


Suggested change

tag: "2.1.5",

revision: "18a3f713267b2a08e34cbe04253df891889997a2"

tag: "2.1.5",

revision: "18a3f713267b2a08e34cbe04253df891889997a2"

Align the arguments of url. Though, we really should use a tarball instead of a Git checkout.

carlocab · 2024-05-22T17:45:21Z

Formula/n/newnode.rb

+    depends_on "gnu-sed" => :build
+    depends_on "grep" => :build


Suggested change

depends_on "gnu-sed" => :build

depends_on "grep" => :build

It's unlikely that these are needed, since sed and grep are POSIX commands.

carlocab · 2024-05-22T17:45:45Z

Formula/n/newnode.rb

+  on_linux do
+    depends_on "gnu-sed" => :build
+    depends_on "grep" => :build
+    depends_on "llvm" => :build


Makes this

uses_from_macos "llvm" => :build

(outside of the on_linux block.

carlocab · 2024-05-22T17:46:30Z

Formula/n/newnode.rb

+    if OS.linux?
+      inreplace "https_wget.c", "/usr/bin/wget", "#{Formula["wget"].opt_bin}/wget"
+      ENV.prepend_path "PATH", Formula["gnu-sed"].opt_libexec/"gnubin"
+    end


Suggested change

if OS.linux?

inreplace "https_wget.c", "/usr/bin/wget", "#{Formula["wget"].opt_bin}/wget"

ENV.prepend_path "PATH", Formula["gnu-sed"].opt_libexec/"gnubin"

end

inreplace "https_wget.c", "/usr/bin/wget", "#{Formula["wget"].opt_bin}/wget" if OS.linux?

unless brew complains about the line being too long, in which case,

Suggested change

if OS.linux?

inreplace "https_wget.c", "/usr/bin/wget", "#{Formula["wget"].opt_bin}/wget"

ENV.prepend_path "PATH", Formula["gnu-sed"].opt_libexec/"gnubin"

end

if OS.linux?

inreplace "https_wget.c", "/usr/bin/wget", "#{Formula["wget"].opt_bin}/wget"

end

carlocab · 2024-05-22T17:46:45Z

Formula/n/newnode.rb

+    path = (var/"cache/newnode")
+    path.mkpath
+    path.chmod 0775


This should probably be in a post_install method.

carlocab · 2024-05-22T17:47:51Z

Formula/n/newnode.rb

+    begin
+      system "curl", "https://brew.sh"
+    ensure
+      sleep 5
+      Process.kill("TERM", pid)
+    end


Suggested change

begin

system "curl", "https://brew.sh"

ensure

sleep 5

Process.kill("TERM", pid)

end

system "curl", "https://brew.sh"

sleep 5

ensure

Process.kill("TERM", pid)

I don't think you need the begin block here.

theoden8 · 2024-05-24T22:02:23Z

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 · 2024-06-04T11:16:15Z

@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.

SMillerDev · 2024-06-04T12:10:10Z

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.

newnode-helper 2.1.4 (new formula)

cc01a26

github-actions bot added the new formula PR adds a new formula to Homebrew/homebrew-core label Feb 14, 2024

chenrui333 reviewed Feb 14, 2024

View reviewed changes

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

chenrui333 reviewed Feb 14, 2024

View reviewed changes

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

chenrui333 reviewed Feb 14, 2024

View reviewed changes

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

filled in the license

9d53b49

github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Feb 14, 2024

removed extra cloning

1003532

SMillerDev reviewed Feb 14, 2024

View reviewed changes

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

SMillerDev requested changes Feb 14, 2024

View reviewed changes

p-linnane requested changes Feb 21, 2024

View reviewed changes

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

theoden8 added 3 commits February 21, 2024 22:58

removed wget dependency

4984c90

further inspection of https://github.com/clostra/newnode/blob/master/build.sh and subsequent build tests confirm that wget isnt needed on mac os

fork proxy in test

669f2db

instead of assuming that the proxy is already run as a service, run it in a fork and kill at completion

disable linux builds for now

3d132b8

linux builds currently rely on a hard-coded wget binary, which needs to change upstream to non-hardcoded binary

github-actions bot added the macos-only Formula depends on macOS label Feb 22, 2024

theoden8 added 3 commits February 22, 2024 00:51

fix ordering

3fdbb12

fix brew audit fail due to non-alphabetic ordering of dependencies

added build dependencies

5777bd3

most of the dependencies specified so far are actually build-time only, and pkg-config was not specified at all

update to 2.1.5, no need for mbedtls

1ece97d

updated newnode-helper to 2.1.5, with updated dependencies and no dependency on mbedtls

theoden8 changed the title ~~newnode-helper 2.1.4 (new formula)~~ newnode-helper 2.1.5 (new formula) Feb 29, 2024

theoden8 requested review from p-linnane and SMillerDev March 1, 2024 09:48

SMillerDev reviewed Mar 4, 2024

View reviewed changes

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved

theoden8 and others added 2 commits March 4, 2024 14:25

Update Formula/n/newnode-helper.rb

a726984

Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>

renamed newnode-helper.rb to newnode.rb

032c4d8

theoden8 added 2 commits March 26, 2024 13:44

fix cache mkpath

6fd2073

enable linux and replace wget with homebrews wget

bef4833

github-actions bot removed the macos-only Formula depends on macOS label Mar 26, 2024

theoden8 added 3 commits March 26, 2024 14:04

fix audit

480e96e

fix linux builds

83028ed

fix audit

69533d0

p-linnane removed their request for review March 26, 2024 16:12

SMillerDev reviewed Mar 27, 2024

View reviewed changes

Formula/n/newnode.rb Outdated Show resolved Hide resolved

Formula/n/newnode.rb Outdated Show resolved Hide resolved

Update Formula/n/newnode.rb

73f531a

Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>

SMillerDev reviewed Apr 11, 2024

View reviewed changes

chenrui333 reviewed Apr 11, 2024

View reviewed changes

Formula/n/newnode.rb Outdated Show resolved Hide resolved

theoden8 added 4 commits April 11, 2024 13:31

formatted revision

f7160d8

no need to double kill

b57c191

fix audit

d282a5e

fails_with :gcc

42a477f

SMillerDev requested review from p-linnane and SMillerDev May 22, 2024 08:10

p-linnane requested review from a team and removed request for p-linnane May 22, 2024 17:27

carlocab requested changes May 22, 2024

View reviewed changes

carlocab closed this May 22, 2024

github-actions bot added the outdated PR was locked due to age label Jul 7, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

newnode 2.1.5 (new formula) #162683

newnode 2.1.5 (new formula) #162683

theoden8 commented Feb 14, 2024 •

edited

Loading

github-actions bot commented Feb 14, 2024

SMillerDev left a comment

theoden8 commented Feb 14, 2024 •

edited

Loading

theoden8 commented Feb 21, 2024 •

edited

Loading

SMillerDev commented Feb 21, 2024

theoden8 commented Feb 21, 2024 •

edited

Loading

p-linnane left a comment

theoden8 commented Feb 29, 2024 •

edited

Loading

theoden8 commented Apr 9, 2024

SMillerDev Apr 11, 2024

theoden8 Apr 11, 2024

theoden8 commented May 7, 2024

theoden8 commented May 21, 2024

p-linnane commented May 22, 2024

carlocab left a comment

carlocab May 22, 2024

carlocab May 22, 2024

carlocab May 22, 2024

carlocab May 22, 2024

carlocab May 22, 2024

carlocab May 22, 2024

carlocab May 22, 2024

theoden8 commented May 24, 2024 •

edited

Loading

theoden8 commented Jun 4, 2024 •

edited

Loading

SMillerDev commented Jun 4, 2024

		tag: "2.1.5",
		revision: "18a3f713267b2a08e34cbe04253df891889997a2"

newnode 2.1.5 (new formula) #162683

newnode 2.1.5 (new formula) #162683

Conversation

theoden8 commented Feb 14, 2024 • edited Loading

github-actions bot commented Feb 14, 2024

SMillerDev left a comment

Choose a reason for hiding this comment

theoden8 commented Feb 14, 2024 • edited Loading

theoden8 commented Feb 21, 2024 • edited Loading

SMillerDev commented Feb 21, 2024

theoden8 commented Feb 21, 2024 • edited Loading

p-linnane left a comment

Choose a reason for hiding this comment

theoden8 commented Feb 29, 2024 • edited Loading

theoden8 commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theoden8 commented May 7, 2024

theoden8 commented May 21, 2024

p-linnane commented May 22, 2024

carlocab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theoden8 commented May 24, 2024 • edited Loading

theoden8 commented Jun 4, 2024 • edited Loading

SMillerDev commented Jun 4, 2024

theoden8 commented Feb 14, 2024 •

edited

Loading

theoden8 commented Feb 14, 2024 •

edited

Loading

theoden8 commented Feb 21, 2024 •

edited

Loading

theoden8 commented Feb 21, 2024 •

edited

Loading

theoden8 commented Feb 29, 2024 •

edited

Loading

theoden8 commented May 24, 2024 •

edited

Loading

theoden8 commented Jun 4, 2024 •

edited

Loading