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

mikutter 3.4.6 (new formula) #6258

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@midchildan
Contributor

midchildan commented Oct 25, 2016

  • Have you followed the guidelines for contributing?
  • 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?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

mikutter is an extensible twitter client. It is also available in the official repos of Debian, Gentoo, and BSDs.

"--install-dir", "#{libexec}/vendor")
end
prefix.install_metafiles

This comment has been minimized.

@dunn

dunn Oct 26, 2016

Contributor

If you're not changing the working directory during installation, this happens automatically.

This comment has been minimized.

@midchildan

midchildan Oct 26, 2016

Contributor

You mean prefix.install_metafiles ?

This comment has been minimized.

@bfontaine
end
prefix.install_metafiles
rm_rf "vendor"

This comment has been minimized.

@dunn

dunn Oct 26, 2016

Contributor

This shouldn't be necessary.

This comment has been minimized.

@midchildan

midchildan Oct 26, 2016

Contributor

The vendor directory initially contains binaries for linux, so it is necessary to remove them before doing libexec.install Dir["*"]

libexec.install Dir["*"]
if build.with? "terminal-notifier"
mkpath libexec/"plugin"

This comment has been minimized.

@dunn

dunn Oct 26, 2016

Contributor

#write will create the path if necessary, so this and the rest of the mkpaths aren't needed: https://github.com/Homebrew/brew/blob/master/Library/Homebrew/extend/pathname.rb#L141

end
def exec_script
<<-EOS.undent

This comment has been minimized.

@dunn

dunn Oct 26, 2016

Contributor

Is this a wrapper you wrote yourself, or is it supplied by upstream?

This comment has been minimized.

@dunn

dunn Oct 26, 2016

Contributor

(Likewise for the compatibility plugin below)

This comment has been minimized.

@midchildan

midchildan Oct 26, 2016

Contributor

I wrote these wrappers myself, since the upstream project mainly targets Linux.

This comment has been minimized.

@bfontaine

bfontaine Oct 26, 2016

Member

You should submit them to upstream instead.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 26, 2016

Member

You should submit them to upstream instead.

Agreed. Upstream need to be willing to support macOS.

This comment has been minimized.

@midchildan

midchildan Oct 26, 2016

Contributor

I actually considered submitting them to upstream, but the wrappers depends on HOMEBREW_PREFIX, making it a little difficult.

This comment has been minimized.

@bfontaine

bfontaine Oct 26, 2016

Member

You don’t have to depend on HOMEBREW_PREFIX; just use the binaries available in the PATH.

This comment has been minimized.

@midchildan

midchildan Oct 27, 2016

Contributor

I could do that, but I would still need HOMEBREW_PREFIX for reasons given below.

depends_on "gtk+"
depends_on "terminal-notifier" => :recommended
depends_on :ruby => "2.1+"

This comment has been minimized.

@dunn

dunn Oct 26, 2016

Contributor

The RubyRequirement doesn't use the +, it should just be "2.1" and that means 2.1 or higher.

@MikeMcQuaid

This comment has been minimized.

Member

MikeMcQuaid commented Oct 26, 2016

As a general comment: this formula is kinda a full port to macOS. I'd strongly recommend workign with upstream to get all this stuff (including e.g. icns generation) upstream and ideally get a .app bundle that they can build and support.

metadata[:message] = text
command = [
"#{HOMEBREW_PREFIX}/opt/terminal-notifier/bin/terminal-notifier",

This comment has been minimized.

@bfontaine

bfontaine Oct 26, 2016

Member

You can’t rely on this being present because it’s only :recommended; not required.

This comment has been minimized.

@bfontaine

bfontaine Oct 26, 2016

Member

Even if you could you should use terminal-notifier instead of its full path.

@midchildan

This comment has been minimized.

Contributor

midchildan commented Oct 27, 2016

@MikeMcQuaid The compatibility plugin in this formula could easily be provided by upstream or elsewhere, but it is not so easy for the rest. First, mikutter depends on formulas provided by Homebrew, and you will have to include every single one of them to package it as a standalone .app bundle. I'm not sure upstream will be willing to do that. Second, mikutter needs Hombrew-specific info such as the HOMEBREW_PREFIX. This is necessary because apps launched from the Finder will not include Homebrew's PATH by default.
But the approach I used to make an .app bundle is applicable to other formulas as well. Most of the wrapper script I wrote for launching mikutter is about dealing with differences in the way macOS and other common unix systems handle locales. Specifically, many unix apps depends on LANG being set to a UTF-8 locale and will not work if LANG is unspecified. Can't we perhaps add a helper method for Hombrew to make a .app bundle to wrap unix GUI apps?

@midchildan

This comment has been minimized.

Contributor

midchildan commented Oct 27, 2016

I opened a PR in Homebrew/brew#1383

@midchildan midchildan force-pushed the midchildan:mikutter branch 5 times, most recently from ae80daa to 3aa2a21 Oct 29, 2016

@midchildan

This comment has been minimized.

Contributor

midchildan commented Oct 30, 2016

Fixed formula.

@midchildan midchildan force-pushed the midchildan:mikutter branch from 3aa2a21 to f75b11a Oct 30, 2016

resource "rake" do
url "https://rubygems.org/gems/rake-10.5.0.gem"
sha256 "2b55a1ad44b5c945719d8a97c302a316af770b835187d12143e83069df5a8a49"
end

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Nov 6, 2016

Member

Can you add a blank line between each of these? Thanks!

This comment has been minimized.

@midchildan

@midchildan midchildan force-pushed the midchildan:mikutter branch from f75b11a to c5ce37e Nov 6, 2016

@MikeMcQuaid

This comment has been minimized.

Member

MikeMcQuaid commented Nov 7, 2016

Thanks for your first contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock!

@midchildan midchildan deleted the midchildan:mikutter branch Nov 7, 2016

fabrik42 added a commit to fabrik42/homebrew-core that referenced this pull request Nov 8, 2016

mikutter 3.4.6 (new formula)
Closes Homebrew#6258.

Signed-off-by: Mike McQuaid <mike@mikemcquaid.com>

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.