-
-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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
vis 0.2 #127
vis 0.2 #127
Conversation
Was just about to build one myself. This looks pretty clean. Very interested in trying this out on OS X but I prefer all my packages to be managed through brew. Any ETA on this being merged? |
How about an ETA of "now"? Does "now" work for you? :) Merged. Thank you for your contribution to Homebrew! For future reference, please squash your commits to one commit per formula when you end up doing edits to a PR. (I squashed them while doing the merge this time.) |
Closed by 4b0765d |
Thank you 👍 |
Sorry, in hindsight this wasn't quite ready to merge. Primarily the cause is that this tool conflicts with the system-supplied I actually already fell over this personally trying to write a better test here and wondering why the The other issue is that the test doesn't fit our style. I understand there's a limited amount you can do with a text editor but even if we stick to the same limited theme it should have been: test do
assert_match version.to_s, shell_output("#{bin}/vis -v 2>&1", 1)
end Or similar. |
That was my fault. I jumped the gun on merging even though I had seen the conflicting |
Fixed testing. I tentatively suggest renaming to |
I like Maybe it's worth asking the upstream In addition to Actually—this could affect self-calls, scripting, and process detection, too. vi is an interactive, scriptable, nontrivial program. We should really do some research and ask upstream about this before just renaming a couple binaries. Sorry, looks like ETA is going to be a bit further out than I originally thought. |
Seems like the developer is aware of the conflict and prefers to stay the course (martanne/vis#101). Understandable I suppose. Probably the best option is keg-only then. |
It seems this PR has been revert, what happen? |
ea2ee83
to
b5c114b
Compare
Made it keg-only. This led to an additional problem with vis being unable to locate its startup scripts, necessitating the creation of a shim to set the environment. |
If you plan on doing this often, you may wish to add an alias | ||
to your shell startup file: | ||
|
||
alias vise=$(brew --prefix vis)/bin/vise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a wrapper script you could perhaps just make libexec
the prefix, add the wrapper script and not make it keg-only?
Okay, that seems to work. Still no working man page but oh well. |
Not sure about renaming the package. How about only linking if the user chooses to? |
That's essentially what keg-only is; I think it's a pretty lousy user experience if you just want your editor to work. If the author(s) has/have strong feelings about renaming the binary, we'll respect that but probably also decline to distribute the software. If that ends up being the case, someone could elect to maintain their own tap for the formula. The linked document has some advice and we'd be happy to field questions. @martanne, do you have a preference? |
Disclaimer: I know almost nothing about homebrew/Mac OS X. I think renaming the binary is the wrong thing to do. I thought users would be able to chose/override the vis binary with Also the old formula by @twe4ked explicitly added a dependency on The user experience of homebrew could probably be improved by properly handling Lua dependencies ... |
I wasn't aware that Homebrew could enforce external package dependencies. Formula updated. I'm not inclined to manage a tap for this, so if that is the consensus and someone reading this feels like doing so, feel free. |
Mostly they are libraries which are dependencies of other things which Homebrew distributes. Few of them are applications.
We don't allow it in core; we prefer that formulas download and install modules like this to a private prefix and arrange to make them available when the executables are invoked. In the Python world,
This is certainly true and we would love to work with an interested contributor who understands the Lua ecosystem well; we do not have any at present. |
Thanks for your efforts, @terhorst! |
Renaming to If you want to follow the NetBSD package, the changes required would be:
whether the last two points are necessary is up to you I guess. I could also introduce some The LPeg dependency should be handled in one form or another. |
Yeah, let's go with Renaming Adding some |
I added two |
Great thanks @martanne. Adding these as a patch and reopening might be a good option? |
I have updated the formula to take advantage of these #defines. It is HEAD only until a new version is issued which incorporates these patches. @martanne If you could issue a release 0.2.1 (or something) that I can point to in the formula then I think we'd be done. |
@terhorst @martanne I have updated this formula to build version 0.4. I ran into a problem with the configure script - it seems that if termkey is available then the configure script assumes ncursesw is also present. I "fixed" that by removing the reference to ncursesw from the configure script (but perhaps ncursesw should be added as a dependency instead?). The script is updated to satisfy class Vis < Formula
desc "Vim-like text editor"
homepage "https://github.com/martanne/vis"
url "https://github.com/martanne/vis/archive/v0.4.tar.gz"
sha256 "f11ba41cfb86dd39475960abfd12469de4da0ccfdb941f1d7680d89d987694c5"
head "https://github.com/martanne/vis.git"
depends_on "lua" => :build
depends_on "libtermkey"
resource "lpeg" do
url "https://luarocks.org/manifests/gvvaughan/lpeg-1.0.1-1.src.rock", :using => :nounzip
sha256 "149be31e0155c4694f77ea7264d9b398dd134eca0d00ff03358d91a6cfb2ea9d"
end
def script; <<-EOS.undent
#!/bin/sh
VIS_BASE=`brew --prefix vis`/libexec
VIS_PATH=$VIS_BASE/share/vis $VIS_BASE/bin/vis $@
EOS
end
def install
luapath = libexec/"vendor"
ENV["LUA_PATH"] = "#{luapath}/share/lua/5.2/?.lua"
ENV["LUA_CPATH"] = "#{luapath}/lib/lua/5.2/?.so"
resources.each do |r|
r.stage do
system "luarocks", "build", r.name, "--tree=#{luapath}"
end
end
# avoid inclusion of -lncursesw alongside -ltermkey because lncursesw might not exist (also avoid use of lncursesw altogether to simplify things)
inreplace "configure" do |s|
s.gsub!("for libcurses in ncursesw ncurses libcurses; do", "for libcurses in ncurses libcurses; do")
s.gsub!("LDFLAGS_TERMKEY=\"-ltermkey -lncursesw\"", "LDFLAGS_TERMKEY=\"-ltermkey\"")
end
system "./configure", "--prefix=#{libexec}"
system "make", "install"
(bin/"vise").write script
end
def caveats; <<-EOS.undent
To avoid a name conflict with the OS X system utility /usr/bin/vis,
this text editor must be invoked by calling `vise` ("vis-editor").
EOS
end
test do
assert_match version.to_s, shell_output("#{bin}/vise -v 2>&1", 1)
end
end
|
The incorrect nature of that assumption should be reported & ideally fixed upstream, but essentially if you have a working formula it's better to file a new PR with that instead of bumping old threads. It's easier for ILZ et al. to review things that way rather than hoping they spot the new comment. |
brew audit --strict --online <formula>
(where<formula>
is the name of the formula you're submitting)?brew install <formula>
?