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

newnode 2.1.5 (new formula) #162683

Closed
wants to merge 23 commits into from
Closed

Conversation

theoden8
Copy link

@theoden8 theoden8 commented Feb 14, 2024

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

@github-actions github-actions bot added the new formula PR adds a new formula to Homebrew/homebrew-core label Feb 14, 2024
@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Feb 14, 2024
Copy link
Contributor

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 Outdated Show resolved Hide resolved
Formula/n/newnode-helper.rb Outdated Show resolved Hide resolved
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

theoden8 commented Feb 14, 2024

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
Copy link
Author

theoden8 commented Feb 21, 2024

@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
Copy link
Member

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
Copy link
Author

theoden8 commented Feb 21, 2024

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?

Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
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
@github-actions github-actions bot added the macos-only Formula depends on macOS label Feb 22, 2024
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 theoden8 changed the title newnode-helper 2.1.4 (new formula) newnode-helper 2.1.5 (new formula) Feb 29, 2024
@theoden8
Copy link
Author

theoden8 commented Feb 29, 2024

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?

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
Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>
@github-actions github-actions bot removed the macos-only Formula depends on macOS label Mar 26, 2024
@p-linnane p-linnane removed their request for review March 26, 2024 16:12
Formula/n/newnode.rb Outdated Show resolved Hide resolved
Formula/n/newnode.rb Outdated Show resolved Hide resolved
Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>
@theoden8
Copy link
Author

theoden8 commented Apr 9, 2024

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

Comment on lines 59 to 66
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the second kill

Formula/n/newnode.rb Show resolved Hide resolved
Formula/n/newnode.rb Outdated Show resolved Hide resolved
@theoden8
Copy link
Author

theoden8 commented May 7, 2024

Anything else that need be addressed? @SMillerDev

@theoden8
Copy link
Author

Any further updates on this? @SMillerDev

@p-linnane
Copy link
Member

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

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

@carlocab carlocab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

Comment on lines +15 to +17
on_macos do
depends_on xcode: ["9.3", :build]
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the on_macos block here.

Comment on lines +5 to +6
tag: "2.1.5",
revision: "18a3f713267b2a08e34cbe04253df891889997a2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +20 to +21
depends_on "gnu-sed" => :build
depends_on "grep" => :build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

on_linux do
depends_on "gnu-sed" => :build
depends_on "grep" => :build
depends_on "llvm" => :build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes this

uses_from_macos "llvm" => :build

(outside of the on_linux block.

Comment on lines +31 to +34
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +37 to +39
path = (var/"cache/newnode")
path.mkpath
path.chmod 0775
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be in a post_install method.

Comment on lines +58 to +63
begin
system "curl", "https://brew.sh"
ensure
sleep 5
Process.kill("TERM", pid)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@carlocab carlocab closed this May 22, 2024
@theoden8
Copy link
Author

theoden8 commented May 24, 2024

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
Copy link
Author

theoden8 commented Jun 4, 2024

@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
Copy link
Member

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.

@github-actions github-actions bot added the outdated PR was locked due to age label Jul 7, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosquash Automatically squash pull request commits according to Homebrew style. new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants