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

Vendor composer dependencies here, drop submodule #28509

Closed
wants to merge 4 commits into from

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Aug 19, 2021

For later reference composer require aws/aws-sdk-php:^3.35 bantu/ini-get-wrapper:v1.0.1 christophwurst/id3parser:^0.1.1 deepdiver/zipstreamer:2.0.0 deepdiver1975/tarstreamer:v2.0.0 doctrine/dbal:3.0.0 egulias/email-validator:2.1.25 giggsey/libphonenumber-for-php:^8.12 guzzlehttp/guzzle:^7.2 icewind/searchdav:^2.0.0 icewind/streams:v0.7.5 league/flysystem:^1.0 microsoft/azure-storage-blob:^1.5 nextcloud/lognormalizer:^1.0 nikic/php-parser:^4.2 opis/closure:^3.6 pear/archive_tar:^1.4.9 pear/pear-core-minimal:^v1.10 php-ds/php-ds:^1.3 php-http/guzzle7-adapter:^1.0.0 php-opencloud/openstack:^3.1 phpseclib/phpseclib:2.0.32 pimple/pimple:^3.4.0 psr/container:^1.1.1 psr/event-dispatcher:^1.0 punic/punic:^1.6 sabre/dav:^4.1.3 scssphp/scssphp:^1.4.0 stecman/symfony-console-completion:^0.11.0 swiftmailer/swiftmailer:^6.0 symfony/console:4.4.25 symfony/event-dispatcher:4.4.25 symfony/event-dispatcher-contracts:1.1.9 symfony/polyfill-intl-grapheme:^1.20 symfony/polyfill-intl-normalizer:^1.20 symfony/process:4.4.25 symfony/routing:4.4.25 symfony/translation:4.4.25 web-auth/webauthn-lib:^3.1 is the magic command to "copy" current 3rdparty packages and their versions

@ChristophWurst ChristophWurst added this to the Nextcloud 23 milestone Aug 19, 2021
@ChristophWurst ChristophWurst self-assigned this Aug 19, 2021
@ChristophWurst ChristophWurst requested a review from rullzer August 19, 2021 10:10
@skjnldsv
Copy link
Member

image

🐘

@CarlSchwan
Copy link
Member

I'm a bit concerned about how big the main server repo is becoming. It already takes ages to clone and this will discourage people to contribute if it takes half an hour to clone it. Wouldn't it make more sense to just vendor the dependencies inside the tarballs?

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Aug 19, 2021

I'm a bit concerned about how big the main server repo is becoming. It already takes ages to clone and this will discourage people to contribute if it takes half an hour to clone it. Wouldn't it make more sense to just vendor the dependencies inside the tarballs?

Absolutely fair point. There have been enough voices in the past that raised that issue but it wasn't picked up because Nextcloud is/was supposed to work with just a git clone. AFAIK most of the repo sizes comes from the js bundles, as they are a few MBs with every compilation, then you have people who do lots of commits per PR and eventually the number or large objects in git is causing the repo to grow. Due to the fact that git doesn't ever discard any of its history, the repo will also only grow, not shrink.

I'm all in for not checking in artifacts, but that decision is beyond my influence.

Ideally neither js build artifacts nor any dependencies should be here.

In all fairness I do see that this vendored composer directory adds some bloat to the repo, but it's actually not that much compared to js bundles, because those php dependencies do not change often. It's more of a one time bloat. Further updates will be rather small.

Just learned that js bundles will be dropped, so I assume this change isn't acceptable.

@ChristophWurst ChristophWurst deleted the refactor/no-more-submodule branch August 19, 2021 15:10
@ChristophWurst ChristophWurst removed their assignment Aug 19, 2021
@ChristophWurst
Copy link
Member Author

Wouldn't it make more sense to just vendor the dependencies inside the tarballs?

Just for completeness this is currently not an option from a security point of view. Composer doesn't verify the integrity of packages (at least it didn't do that with v1, might need another evaluation). So anyone who maintains a package that is used in Nextcloud can replace its code at any time, because packagist allows you to overwrite tags, and therefore introduce (malicious) code into Nextcloud without our notice.

What we need for acceptable dependencies that are not vendored but only installed locally and during packaging is that we have some sort of integrity check that is generated when packages are installed. We then need to keep that file maintained with all updates. When server is bundled for a release you install dependencies and can check if the hashes match the expected ones. Only then you ship the code that was verified earlier. cc @LukasReschke

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

Successfully merging this pull request may close these issues.

4 participants