piotrmurach / tty

Toolkit for developing sleek command line apps.
https://ttytoolkit.org
MIT License
2.5k stars 78 forks source link

fatal: Not a git repository (or any of the parent directories): .git #44

Closed quirinux closed 6 years ago

quirinux commented 6 years ago

Not sure if it's properly an error on TTY once it happens on ruby-2.6.0dev, but requiring tty lib prints a git error out, even when -W0 flag is set it makes no difference:

Ruby version: ➜ test ruby --version
ruby 2.6.0dev (2018-07-06 trunk 63870) [x86_64-linux]

Weird git message: ➜ test ruby -W0 -e "require 'tty'; p 'ok'" fatal: Not a git repository (or any of the parent directories): .git "ok"

RVM version: ➜ test rvm --version rvm 1.29.4 (latest) by Michal Papis, Piotr Kuczynski, Wayne E. Seguin [https://rvm.io]

quirinux commented 6 years ago

btw, same here with ruby 2.4.1

quirinux commented 6 years ago

Found the issue, it only happens when requiring tty package and do not happen when requiring a sub-package like tty-table for example, checking the tty.gemspec file at line 17:

spec.files = `git ls-files`.split($/)

Looks like the issue is right there, checking on this conversation on bundler package: https://github.com/bundler/bundler/issues/2039#issuecomment-10128200 They found a better way to sort out those files like:

spec.files = Dir['lib/**/*.rb'] + Dir['bin/*']
spec.files += Dir['[A-Z]*'] + Dir['test/**/*']
spec.files.reject! { |fn| fn.include? "CVS" }

so I simple did on my local gemspec:

  #spec.files         = `git ls-files`.split($/)
  spec.files = Dir['lib/**/*.rb'] + Dir['bin/*']
  spec.files += Dir['[A-Z]*'] + Dir['test/**/*']
  spec.files.reject! { |fn| fn.include? "CVS" }

and dan-dah, it worked like a charm, I'm gonna open a PR for that

piotrmurach commented 6 years ago

Hi,

Thanks for trying out tty gem!

First of all, as a general point you shouldn't be really using require 'tty'. This library is a meta gem for scaffolding terminal apps. It is not meant to be used as a direct requirement. In future, I'm planning on having a manifest file that pulls in latest tty packages and injects them into scaffolded application without actually requiring them directly in gemspec. I would encourage you to directly specify and use the tty gems you need, mix and match! Similar to coinpare

When you require the tty file it loads all the plugins by reading them from the tty.gemspec and I can only assume evaluating the spec.files= assignment in the process. On the other hand, the tty-table uses exactly the same code as hundreds of Ruby gems generated with bundler see tty-table gemspec with a difference that it doesn't evalute gemspec when required, only on installation.

Having read https://github.com/bundler/bundler/pull/2023 & https://github.com/bundler/bundler/issues/1043 I can see both sides of the issue. There is certainly a good case for dropping reliance on external utility such as git to ensure that things with tty can be packaged easily. It would be prudent to ensure that no necessary files are skipped in the process such as dotfiles etc...

I do appreciate you bringing this to my attention and working on the fix. I'm not saying this is not the solution that I will end up using, but I need a little bit of time to investigate and make my mind up. In the meantime let's continue the dialogue and work on the solution.

quirinux commented 6 years ago

perfect @piotrmurach totally agreed, one of my solutions I use a good bunch of tty libs, so it's easier require tty straight forward, but have replaced by each gem I meant to use, indeed got ruby vm bootup time reduced, so the best option is really requiring only needed gems, haven't checked memory footprint yet

About the fix, I submitted a PR, I know it's a minor issue and the changes I made can impact in the whole solution ans user experience, so feel free to reject it or, even better, add your comments here or on the PR and I make the needed changes

piotrmurach commented 6 years ago

Hey @quirinux I've changed gemspec to read a manifest file in order to figure out which files to include which heavily relies on your findings.

Thanks for bringing this to my attention and coming up with a solution! ❤️ I will release shortly.

quirinux commented 6 years ago

it is a such good idea, better than mine, cheers