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

nimbus-eth2 1.5.5 (new formula) #91969

Closed
wants to merge 1 commit into from
Closed

Conversation

riptl
Copy link
Contributor

@riptl riptl commented Dec 23, 2021

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

nimbus-eth2 is an implementation of the Eth2 beacon chain in Nim. Eth2 is a series of ongoing upgrades to the Ethereum cryptocurrency to fix the high energy usage of the network.

This PR is part of a series of new formulas. There exist a few implementations of Eth2, most notably Prysm (Go), Nimbus-Eth2 (Nim), Lighthouse (Rust) #91984 and Teku (Java) #91986. All of these clients are well maintained, implement the same protocol and have mostly the same ergonomics. I aim to enroll all of them in Homebrew.

@BrewTestBot BrewTestBot added the new formula PR adds a new formula to Homebrew/homebrew-core label Dec 23, 2021
@riptl riptl mentioned this pull request Dec 24, 2021
6 tasks
Formula/nimbus-eth2.rb Outdated Show resolved Hide resolved
depends_on "cmake" => :build

def install
system "make"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Makefile pulls in an entire Nim compiler toolchain, instead of using one provided by Homebrew. This should be fixed upstream before creating a formula.

Copy link

Choose a reason for hiding this comment

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

Can you clarify why is this considered a problem? I can imagine various motives:

  1. All downloads of a recipe should be explicitly described through some homebrew-specific API, so they can be verified (against a checksum) and the recipe can be potentially executed in a network-isolated/offline environment.

  2. All intermediate build files should be easy to clean up after the build and the Nim compiler dependency doesn't meet this criteria.

  3. Building your compiler toolchain as part of your build is just considered a sub-optimal approach (i.e. an excessive waste of time).

I'd like to help in resolving the perceived issues, but first I need to understand your concerns better. I doubt Nimbus is the first project that builds a custom tool required only during the build process and I would argue that Nimbus doesn't depend on Nim at run-time, so we don't need to introduce a dependency on Nim in the Homebrew recipe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zah Thank you for taking a look at this. The acceptance criteria are here: https://docs.brew.sh/Acceptable-Formulae

Specifically

We don’t like install scripts that download unversioned things

If I understand correctly, the nimbus_build_system is built from master via a Git submodule.

A Homebrew maintainer is more qualified to comment on this

Copy link

Choose a reason for hiding this comment

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

Through the use of git submodules, we precisely pin the version of the Nim compiler being used as part of our build. We take reproducible builds very seriously, so this applies to all other components as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SMillerDev would you mind taking a look at this? (context: pulling in the compiler via Git submodule)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link

@zah zah Jan 19, 2022

Choose a reason for hiding this comment

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

@carlocab, clearly, the security of Nimbus (a software dealing with finances in the form of cryptocurrencies) is of paramount importance. We, the team behind Nimbus, care very strongly about keeping our dependencies up-to-date with regard to security vulnerabilities and we pin the version of every component used during the build exactly in order to control precisely what goes into the shipped binaries. Here is a relevant except from our docs:

We've designed the build process to be reproducible. In practice, this means that anyone can verify that these exact binaries were produced from the corresponding source code commits. For more about the philosophy and importance of this feature see reproducible-builds.org.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that warrants an exception to our rules.

Please consider hosting it in a personal tap, which is very easy to do: https://docs.brew.sh/How-to-Create-and-Maintain-a-Tap

@riptl
Copy link
Contributor Author

riptl commented Dec 27, 2021

According to upstream

any Nim version other than the one we're shipping (which, on rare occasions, might be a temporary fork) is unsupported and should not be used by packagers.
The Nim compiler breaks backwards compatibility often, so you cannot treat it as a C compiler.

The Nim compiler version is pretty closely coupled to the Nimbus project itself. So we can't use the nim package as a build dependency. The build script instead fetches the compiler toolchain from an external source.

@riptl riptl closed this Dec 27, 2021
@zah
Copy link

zah commented Jan 11, 2022

@terorie, shall we re-open this? As discussed in the sub-thread, the concern of pulling a precise version of Nim during the build process shouldn't be a problem.

@riptl riptl reopened this Jan 11, 2022
@riptl
Copy link
Contributor Author

riptl commented Jan 11, 2022

@zah Sounds good, I've also invited you as a collaborator if you need to push directly to this PR: https://github.com/terorie/homebrew-core/invitations

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

@github-actions github-actions bot added the stale No recent activity label Feb 10, 2022
@github-actions github-actions bot closed this Feb 19, 2022
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 23, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants