sebastianbergmann / diff

Diff implementation
BSD 3-Clause "New" or "Revised" License
7.58k stars 85 forks source link

Add .gitattributes files to prevent unneeded files from being shipped… #84

Closed willemstuursma closed 5 years ago

willemstuursma commented 5 years ago

Noticed some unneeded files ending up in my new project.

Test etc. don't need to be shipped when installed through Composer.

sebastianbergmann commented 5 years ago

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it as I disagree with the notion of not shipping tests.

codecov[bot] commented 5 years ago

Codecov Report

Merging #84 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #84   +/-   ##
=========================================
  Coverage     99.29%   99.29%           
  Complexity      211      211           
=========================================
  Files            12       12           
  Lines           566      566           
=========================================
  Hits            562      562           
  Misses            4        4

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d419334...f0c3252. Read the comment docs.

willemstuursma commented 5 years ago

Ok, it is of course your project. I personally find it very wasteful, I have thousands of test files in my vendor folders locally, on CI and sometimes on production servers.

Your projects are installed a particularly high number of times, so you have the option to make a exceptionally big difference here.

Could you enlighten me on your motivation?

sebastianbergmann commented 5 years ago

On the one hand, there is a technical reason that is detailed in https://github.com/sebastianbergmann/phpunit/issues/2300#issuecomment-249176276. Not sure whether this has been addressed in Composer since then (maybe @seldaek can provide some insight here) but I am definitely not going to change autoloading from using a classmap to something else.

On the other hand, there is a process-related reason. @remicollet (correct me if I'm wrong or if things have changed) once told me that Red Hat uses the archives created by GitHub for their continuous integration effort. And if the tests are excluded from export they will not be in these archives and that process will no longer work.

remicollet commented 5 years ago

TLDR .gitattributes is a terrible hack, to fix the lack of a nice installer for PHP (no, composer is NOT an installer, simply a dependency manager)


tests, doc, license, ... are part of the project. Using .gitattributes to exclude them for github snapshot make every developer using other language laugh at us.... because they have a proper installer (cpan, pip, npm...) which know how to mange files, according to their "roles". Even the old PEAR installer was able to do it.

Red Hat uses the archives created by GitHub

Yes, but of course we can use "git clone + tar" instead (what we already do in tons of projects).

This have been discussed ad nauseam. Shame on PHP community for this.

Perhaps someday, someone will start a new "Installer for PHP" project, .

Ocramius commented 5 years ago

composer is NOT an installer, simply a dependency manager

Yes it is also an installer. I also hook into it in many, many apps.

Anyway, using .gitattributes is a perfectly valid approach, considering that the distribution channel that is available is github, and github either gives us git or tarballs according to its internal rules. If the distribution channel changes (unlikely), then you can surely bring up a separate DSL for handling files.

You can also tweak the composer installer to remove autoload-dev paths, if you feel like that's something it should do, although the .gitattributes approach has worked for half a decade without major hiccups.

remicollet commented 5 years ago

I have thousands of test files in my vendor folders locally, on CI and sometimes on production servers.

Seriously ? you use "composer" to deploy on prod ? On dev, use a global installed phpunit or phpunit.phar, you will get a much lighter tree and autoloader

Ocramius commented 5 years ago

Seriously ? you use "composer" to deploy on prod ?

Yes, the assembled image (when using docker, larger projects) uses composer install. For smaller customers, composer install in prod works too.

On dev, use a global installed phpunit or phpunit.phar, you will get a much lighter tree and autoloader

Sorry, but no: each project has its own version of PHPUnit, and for good reasons (good luck pinning down a bug otherwise). This is the usual "statically compiled" vs "dynamically linked" discussion, and the Go approach clearly wins for me, since anything that has to do with dynamic linking is happily isolated into docker containers for good, these days.

sebastianbergmann commented 5 years ago

I am with @remicollet in that I do not like that Composer uses implict packages based on Git tags instead of packages created based on an explicit specification. In that regard, and only in that regard, Composer and Packagist are a step backwards from the PEAR Installer where packages were created based on the explicit information provided in package.xml.

That being said, though, I am open to the idea of ignoring tests/ etc. in composer.json in all my projects. As long as this does not cause any problems such as the one described in https://github.com/sebastianbergmann/phpunit/issues/2300#issuecomment-249176276.

derrabus commented 5 years ago

What problem does it actually solve to exclude tests from the packages? I mean, I rarely work in an environment where the extra disk space consumed by the tests would actually matter.

Really, no offense. I'm actually curious about the motivation behind this.

Ocramius commented 5 years ago

@derrabus in the typical docker production environment, image size accounts for hosting costs, network bandwidth usage, and deployment speed. This is usually not a problem with phpunit itself, because docker containers are built with --no-dev, but it surely applies to the larger PHP ecosystem.

willemstuursma commented 5 years ago

@derrabus you can also think about all the test classes popping up in IDE autocompletion, or Azure Ap Services storage (which is networked).

@Ocramius It could maybe better be solved at the Composer level - an option to scrub all classes files referenced in autoload-dev from disk.

derrabus commented 5 years ago

@willemstuursma While I wouldn't reuse the autoload-dev setting for this, I'd agree that this is a problem that composer should solve if possible, if it's really only about the files that end up on disk and not so much about the size of the dist packages downloaded from github et al.

derrabus commented 5 years ago

There actually is an old composer issue on this topic: composer/composer#5367