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.4 (new formula) #153960

Closed
wants to merge 3 commits into from
Closed

Conversation

flyinhome
Copy link

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

@github-actions github-actions bot added autosquash Automatically squash pull request commits according to Homebrew style. new formula PR adds a new formula to Homebrew/homebrew-core labels Nov 11, 2023
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.

@flyinhome flyinhome closed this Nov 11, 2023
Copy link
Author

@flyinhome flyinhome left a comment

Choose a reason for hiding this comment

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

adds dependency on macos high sierra so it won't try to build on linux

@flyinhome flyinhome reopened this Nov 11, 2023
@chenrui333 chenrui333 changed the title Add newnode helper newnode 2.1.4 (new formula) Nov 11, 2023
@SMillerDev
Copy link
Member

adds dependency on macos high sierra so it won't try to build on linux

Why shouldn't it?

Comment on lines +21 to +51
plist_path = prefix/"newnode-helper.plist"
plist_content = <<~EOS
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>Label</key>
<string>com.clostra.newnode-helper</string>
<key>ProgramArguments</key>
<array>
<string>#{opt_bin}/newnode-helper</string>
<string>-p</string>
<string>8006</string>
<string>-v</string>
</array>
<key>RunAtLoad</key>
<true/>
<key>KeepAlive</key>
<true/>
<key>UserName</key>
<string>newnode</string>
<key>WorkingDirectory</key>
<string>/var/newnode-helper</string>
<key>StandardOutPath</key>
<string>/var/log/newnode-helper.log</string>
<key>StandardErrorPath</key>
<string>/var/log/newnode-helper-error.log</string>
</dict>
</plist>
EOS
plist_path.write(plist_content)
Copy link
Member

Choose a reason for hiding this comment

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

If this isn't provided by upstream we shouldn't ship it

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.

Way too much magic going on in this install

@flyinhome
Copy link
Author

flyinhome commented Nov 12, 2023 via email

@flyinhome
Copy link
Author

adds dependency on macos high sierra so it won't try to build on linux

Why shouldn't it?

Separately from the effort to make newnode-helper easily installable via homebrew, we've set up a snap to facilitate easy installation on linux platforms. We judged (perhaps incorrectly) snap to be more likely to facilitate installs on a wide variety of Linux platforms, and homebrew to be more likely to facilitate installs for MacOS because homebrew is very widely used on MacOS.

We could adapt this homebrew formula to facilitate homebrew installs on Linux also, since the C source code for newnode builds fine on Linux. But at least in my experience things can get confusing for users when the same software package can be installed via multiple mechanisms. For example, when version X of the software is installed via one mechanism and upgraded to version Y via a different mechanism.

So if we wanted to facilitate installs of newnode-helper via both homebrew and snap, we'd want to make sure that all future upgrades (by whatever mechanism) left the resulting installation in a consistent state no matter which mechanism was used to do the upgrade. And that seems tricky, and at least, not what we wanted to try on first attempt.

@flyinhome
Copy link
Author

Just to let people know, the authors' intention for the newnode-helper formula is to update the underlying repo to put more of the "magic" in the newnode repository code, and less of the "magic" in the homebrew formula, and submit another PR once the new combination is tested.

In some ways this is more complicated because it requires coordination among more parties and makes testing more difficult. But we understand that homebrew maintainers want to keep formulae as simple and straightforward as possible.

@SMillerDev
Copy link
Member

We could adapt this homebrew formula to facilitate homebrew installs on Linux also, since the C source code for newnode builds fine on Linux. But at least in my experience things can get confusing for users when the same software package can be installed via multiple mechanisms.

That seems like the best solution since the current split seems very arbitrary. I see the risk of https://xkcd.com/1987/, but that's up to users to fix. Not up to us to restrict.

@flyinhome
Copy link
Author

We could adapt this homebrew formula to facilitate homebrew installs on Linux also, since the C source code for newnode builds fine on Linux. But at least in my experience things can get confusing for users when the same software package can be installed via multiple mechanisms.

That seems like the best solution since the current split seems very arbitrary. I see the risk of https://xkcd.com/1987/, but that's up to users to fix. Not up to us to restrict.

The split isn't arbitrary, since MacOS and Linux are very different platforms, and most MacOS users are accustomed to getting their software from different sources than most Linux users.

Also, what we need for newnode-helper is for the app to run as a daemon full time, and as its own user (least privilege). There's no portable way to do that on Linux. Snap provides a way to do that across a wide variety of Linux platforms. If homebrew on Linux supports the ability to do that on a large number of Linux platforms, please let me know what it is. But I'm under the impression that homebrew really isn't designed for that use case, and it takes "magic" to get it to work even on a Mac.

@SMillerDev
Copy link
Member

The split isn't arbitrary, since MacOS and Linux are very different platforms, and most MacOS users are accustomed to getting their software from different sources than most Linux users.

It's arbitrary because the tool works for both, but as a package manager we're disabling that choice. It's like buying a car in two countries and in one of them the car will refuse to drive on highways.

Also, what we need for newnode-helper is for the app to run as a daemon full time, and as its own user (least privilege). There's no portable way to do that on Linux. Snap provides a way to do that across a wide variety of Linux platforms. If homebrew on Linux supports the ability to do that on a large number of Linux platforms, please let me know what it is. But I'm under the impression that homebrew really isn't designed for that use case, and it takes "magic" to get it to work even on a Mac.

https://docs.brew.sh/Formula-Cookbook#service-files

Copy link
Contributor

github-actions bot commented Dec 6, 2023

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 Dec 6, 2023
@fxcoudert
Copy link
Member

Just to let people know, the authors' intention for the newnode-helper formula is to update the underlying repo to put more of the "magic" in the newnode repository code, and less of the "magic" in the homebrew formula, and submit another PR once the new combination is tested.

Yes, that's definitely the way to go forward. I am closing this inactive PR, but we'll be happy to consider a new one once the install is streamlined. Thanks @flyinhome !

@fxcoudert fxcoudert closed this Dec 10, 2023
@theoden8 theoden8 mentioned this pull request Feb 14, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosquash Automatically squash pull request commits according to Homebrew style. new formula PR adds a new formula to Homebrew/homebrew-core stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants