Skip to content
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

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

Conversation

willemstuursma
Copy link

@willemstuursma willemstuursma commented Feb 21, 2019

Noticed some unneeded files ending up in my new project.

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

@sebastianbergmann
Copy link
Owner

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
Copy link

codecov bot commented Feb 21, 2019

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
Copy link
Author

willemstuursma commented Feb 21, 2019

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
Copy link
Owner

On the one hand, there is a technical reason that is detailed in sebastianbergmann/phpunit#2300 (comment). 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
Copy link
Contributor

remicollet commented Feb 21, 2019

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
Copy link
Sponsor

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
Copy link
Contributor

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
Copy link
Sponsor

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
Copy link
Owner

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 sebastianbergmann/phpunit#2300 (comment).

@derrabus
Copy link

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
Copy link
Sponsor

@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
Copy link
Author

@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
Copy link

@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
Copy link

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

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

Successfully merging this pull request may close these issues.

None yet

5 participants