Skip to content
This repository has been archived by the owner on Aug 8, 2018. It is now read-only.

Move formula into Homebrew/homebrew #115

Closed
fwalch opened this issue Nov 4, 2015 · 33 comments
Closed

Move formula into Homebrew/homebrew #115

fwalch opened this issue Nov 4, 2015 · 33 comments

Comments

@fwalch
Copy link
Member

fwalch commented Nov 4, 2015

Now that there's a release, it should be possible to get the formula accepted into https://github.com/Homebrew/homebrew so we don't have to maintain it ourselves anymore. This might involve having to properly package the third-party dependencies, though.

@justinmk
Copy link
Member

After #195 this is mostly just waiting for someone to send a PR to homebrew.

@equal-l2
Copy link
Contributor

equal-l2 commented Dec 16, 2016

A week has passed since the last commit.

If there's no problem after the last commit, I'll make PR on homebrew-core in several days.

@mhinz
Copy link
Member

mhinz commented Dec 16, 2016

Wasn't there still Lua stuff to sort out? And maybe changing to mpack.

Is there any advantage of having Neovim in homebrew-core apart from the fact that users don't have to use a tap anymore?

@equal-l2
Copy link
Contributor

@mhinz
Nope.
I was too impatient, sorry for bothering.

@mhinz
Copy link
Member

mhinz commented Dec 16, 2016

Well, I didn't mean to sound declining, I really wasn't sure about it. :-) You did some great contributions already, 👍, but staying in control feels right at the moment.

@javian
Copy link
Contributor

javian commented Dec 17, 2016

Why do you feel that it would go out of control by committing it to homebrew ? Its an open community with many maintainers so if things needs to be patched then it will happen quite quickly if PRs are made.
The other advantage would be that there a pre-compiled binaries to download so that user won't need to compile it so it will be faster to deploy from homebrew and that it potentially might meet a wider audience that wouldn't be exposed to neovim otherwise.

@justinmk
Copy link
Member

@javian Good points. Pre-compiled option would be really helpful.

@fxcoudert
Copy link

Hi, I'd like to get this formula into Homebrew, given that it is currently the most-used external tap :)
It would require to use the released luv and lua. Can you help with that?

@justinmk
Copy link
Member

justinmk commented Apr 8, 2017

@fxcoudert Reviewing #198 it seems the blocker is luarocks. We need lpeg and some other lua modules for some of our build scripts.

By the way, what statistics are you looking at?

@fxcoudert
Copy link

I don't understand, as Homebrew lua is built with luarocks support by default.

(Homebrew analytics.)

@justinmk
Copy link
Member

justinmk commented Apr 8, 2017

Maybe @equal-l2 can comment.

@equal-l2
Copy link
Contributor

equal-l2 commented Apr 9, 2017

Neovim uses luajit instead of pure lua because of performance issue, right?

I've never tried with homebrew's lua, but it might be worth to try.

@equal-l2
Copy link
Contributor

equal-l2 commented Apr 9, 2017

We need a means to install rocks (lpeg, mpack, and bitlib) from homebrew formula to fully depend to homebrew's lua.

@justinmk
Copy link
Member

justinmk commented Apr 9, 2017

We need a means to install rocks (lpeg, mpack, and bitlib) from homebrew formula to fully depend to homebrew's lua.

@fxcoudert Is that possible/acceptable in a homebrew formula? Do you know of any formulae doing so, that we can use as an example?

@javian
Copy link
Contributor

javian commented Apr 10, 2017

@justinmk https://github.com/Homebrew/homebrew-core/blob/master/Formula/sile.rb and https://github.com/Homebrew/homebrew-core/blob/master/Formula/corsixth.rb depends on lpeg. Looks like you can't bottle those Formulas though if you depend on lua modules.

@justinmk
Copy link
Member

@javian Thanks! For neovim, the luarocks dependencies are for build-time only, not runtime. I wonder if that still precludes bottling? @fxcoudert

@alyssais
Copy link

Hey! Another Homebrew maintainer here. I haven't looked into it too much, but I'd be very surprised if a build-only Lua dependency prevented bottling.

@justinmk
Copy link
Member

@alyssais thanks. Specifically, we download lua modules via luarocks. Same deal?

@alyssais
Copy link

alyssais commented May 9, 2017

I think so.

@MikeMcQuaid
Copy link

Anyone here (particularly Homebrew maintainers): submit the formula as-is to Homebrew/homebrew-core; we can have better discussion on that than before. Thanks ❤️

@javian
Copy link
Contributor

javian commented Jun 9, 2017

I'm trying at the moment but there seems to be some issues when the last PR is included in the Formula and building HEAD.

@equal-l2
Copy link
Contributor

@javian You can ignore my PR since the formula works well now without the PR.

@javian
Copy link
Contributor

javian commented Jun 25, 2017

So I finally managed to get the formula merged into core. It does not have the same same feature set as the neovim formula since it is meant to produce a production ready binary so I'm not sure what you want to do with this now. I'll commit a PR to manage the migration and the deletion of the Formula here and you can decide what to do with it.

@justinmk
Copy link
Member

@javian What exactly are the feature differences? I would like to retire this repo if possible.

@javian
Copy link
Contributor

javian commented Jun 25, 2017

The differences are that there is no "dev" build build_type = build.with?("dev") ? "Dev" : "RelWithDebInfo" and that the binary is not compiled with RelWithDebInfo but instead uses Release.

It also doesn't use the third-party downloads that are managed through the cmake files but instead tries to utilize the pre-exisiting homebrew formulas as much as possible and downloads the required luarocks as formula resources. This is turn means that some of the versions are not synced like luarocks which in homebrew is bundles with the lua formula which uses an older version (2.3.0) compared to the one that's defined in the cmake scripts.

It shouldn't really make a difference for the end user but might have an impact that you might get less info when people submit bug reports initially.

@javian
Copy link
Contributor

javian commented Jun 25, 2017

And just to give you some numbers to support that here are the installation numbers since April 1st from homebrew.

screen shot 2017-06-25 at 19 04 42

The large majority of the users seems to be using the standard Formula. I just recalled that there is also no option to opt-out of using jemalloc in the core formula since it is assumed that it should work, is the preferred option from the neovim developers and any bugs are related to neovim code (which should be fixed =)).

@justinmk
Copy link
Member

justinmk commented Jun 25, 2017

Thanks for those notes!

The Dev build doesn't matter since neovim/neovim#6827 and will probably be removed from neovim core.

RelWithDebInfo is important and I hope it can be added as an option to the core formula. But maintaining this repo only for that purpose is not going to be worth the user and developer confusion.

no option to opt-out of using jemalloc in the core formula since it is assumed that it should work, is the preferred

Yes, as long as it is pinned to a known good version of jemalloc. jemalloc latest version can break, even in released versions, sometimes due to macOS changes.

@javian
Copy link
Contributor

javian commented Jun 25, 2017

I also forgot to mention that the Formula doesn't support Linux and that it would need to be added to the linuxbrew repo for that to work.

@javian
Copy link
Contributor

javian commented Jun 25, 2017

@justinmk can you elaborate why RelWithDebInfo is important and could you link to any examples where it has been used ?

@justinmk
Copy link
Member

RelWithDebInfo allows users to get backtraces if a crash occurs.

@MikeMcQuaid
Copy link

@justinmk Has that been useful in debugging things for you on macOS specifically? We generally disable debug symbols because they don't work well on macOS when binaries are built in a different location to where they are installed.

@justinmk
Copy link
Member

justinmk commented Jun 26, 2017

@MikeMcQuaid Looks like I was cargo-culting. I see that function names appear in macOS crash reports even without debug builds, and that's all we usually get anyways. So RelWithDebInfo is not needed.

Looks like rust has trouble with this as well: rust-lang/rust#24346 (comment)

@MikeMcQuaid
Copy link

@justinmk Thanks, good to know.

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

No branches or pull requests

8 participants