newrelic-ruby-agent icon indicating copy to clipboard operation
newrelic-ruby-agent copied to clipboard

The .gemspec files' `file_list` arguments include unwanted content

Open fallwith opened this issue 3 years ago • 7 comments

newrelic_rpm.gemspec and infinite_tracing/newrelic-infinite_tracing.gemspec both start with git ls-files and then trim away unwanted content.

We don't appear to be trimming away enough.

For newrelic_rpm.gemspec, we shouldn't be including the files such as the following:

  • test/agent_helper.rb
  • Guardfile
  • DOCKER.md

For each .gemspec file, we should audit the list of files that is being formulated. In addition to the obvious files such as those above, we should also consider removing the following other types of content:

  • rake task content
  • bin/ content
  • build.rb

fallwith avatar May 10 '22 08:05 fallwith

just to be clear, you want to delete the above mentioned?

jmoldyvan avatar May 10 '22 20:05 jmoldyvan

Hi @jmoldyvan! The files specification within a Gem::Specification block is used to produce an allowlist of the repository's files that will be packaged up into the .gem RubyGem file that is published to RubyGems.org.

In our case with this project we have 2 .gemspec files that are used to build our newrelic_rpm and newrelic-inifinite_tracing gems. I'm currently under the impression that we are packaging up a lot of content that is useful for developers wanting to work on developing the gems, but not for Rubyists who are simply wanting to install the gems and use them in their projects. You do not require the DOCKER.md file to simply use New Relic to monitor your Rails application, for example.

So when gem build is ran, we'd like to see if the list of included files can be trimmed down. Either by changing the git ls-files command or by modifying the logic that is applied to that command's output, we should be able to exclude more content than we are now.

fallwith avatar May 10 '22 21:05 fallwith

@fallwith IIRC we want to drop all the files that are not required to run the gem while packaging it. More like when we bundle js we try to reduce the bundle size by dropping files which are not relevant for it to run.

I see quite few files like that, example CHANGELOG.md which can definitely be removed.

Could you share some of such files you feel which are relevant (I am still going through the code) even though they feel not directly relevant to the gem, and could be missed by me? (Like, is LICENSE required?)

manuraj17 avatar May 20 '22 03:05 manuraj17

Hi @manuraj17! Thank you for taking a look at this issue.

We do not have a complete list of files to either include or exclude. We welcome PRs that would propose which files to additionally exclude, and we can always remove some definitely unwanted files with an initial PR and then more files later with follow-up PRs.

There are two different ways to think about which files should be included in the gem:

  • Include only those files needed for the Ruby code itself to operate
  • Include files of interest to a user of the gem, without the content needed to actually develop the gem

That first approach would involve grabbing the files beneath lib/ and bin/ (or exe/ for other projects) and not much else if anything.

The second approach argues that things like the README, the LICENSE, and the CHANGELOG are of interest to the user of a gem and should be included.

We would be open to reviewing PRs that take either approach.

It might be useful to read through some relevant blog posts such as Writing a Ruby Gem Specification by Piotr Murach (author of TTY) and How I write a gemspec by Rodrigo Andrés Vilina (author of Muina).

It might also be useful to review the .gemspec files for other popular gems. The bundler.gemspec file, for example, follows the second approach and does include things like the LICENSE file.

fallwith avatar May 23 '22 21:05 fallwith

@fallwith Alright! Thanks for the detailed reply. Let me go through this and open a PR 👍🏽 Will keep you posted.

manuraj17 avatar May 31 '22 13:05 manuraj17

Let's review the .gemspec files and produce a list of files that should be included to help move the ticket along.

angelatan2 avatar Jun 27 '22 17:06 angelatan2

https://issues.newrelic.com/browse/NEWRELIC-3455

The following files and directories should be prevented from being included:

.gitignore
.project
.rubocop.yml
.rubocop_todo.yml
.simplecov
.snyk
.yardopts
Brewfile
CONTRIBUTING.md
Dockerfile
DOCKER.md
docker-compose.yml
config/
config.dot
Guardfile
lefthook.yml
README.md
test/

The developer facing documentation and Docker content on the list above should be removed given that the unit tests are not available in the .gem distribution. LICENSE, CHANGELOG, and THIRD_PARTY content should stay.

fallwith avatar Jun 13 '23 20:06 fallwith

@fallwith @angelatan2 First of all aplogies for not picking up this immediately. Lot had happened personally and I could not get mind space for contributions.

I have opened a small PR with my planned solution, it's not complete but more like an idea sharing. Please do have a check. TIA.

manuraj17 avatar Jun 20 '23 07:06 manuraj17

Closed by #2089

tannalynn avatar Jun 23 '23 16:06 tannalynn