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

[RFC] Dependency Management #8555

Closed
LorbusChris opened this issue Feb 26, 2018 · 20 comments
Closed

[RFC] Dependency Management #8555

LorbusChris opened this issue Feb 26, 2018 · 20 comments

Comments

@LorbusChris
Copy link
Contributor

LorbusChris commented Feb 26, 2018

I noticed a few things when deving on nextcloud-rpm:

  • 3rdparty/LICENSE INFO is not up to date which means there is no up-to-date listing of all vendored dependencies and their respective licenses.
  • Vendored JS libs and their respective licenses have no listing (only some, but not all js deps can be found in various bower.json files).
  • vendor vs 3rdparty: There is no consistent naming of the vendor dirs in apps/. Sometimes JS deps are in a /js dir, sometimes not:
comments/js/vendor
files_external/3rdparty
gallery/js/vendor
gallery/vendor
pdf_viewer/vendor
texteditor/js/core/vendor
theming/js/3rdparty
user_ldap/vendor

I propose to always put vendored libs in <appname>/3rdparty, regardless if its a JS or PHP lib. That way it'll be much easier to keep track of all bundled libraries.

I also propose to replace LICENSE INFO with 3RDPARTY_INFO containing info about Licenses and Versions of the bundled libs in 3rdparty as well any Patches applied (and Links to their respective Upstream Issues). Do the same in apps/APPS_INFO for any bundled libs in apps (that are shipped with core).

If this is a welcome change, I will create a PR.

@MorrisJobke
Copy link
Member

I propose to always put vendored libs in /3rdparty, regardless if its a JS or PHP lib. That way it'll be much easier to keep track of all bundled libraries.

That is not possible with our current code to load JS files into templates, where we need t load it from js/....

I also propose to replace LICENSE INFO with 3RDPARTY_INFO containing info about Licenses and Versions of the bundled libs in 3rdparty as well any Patches applied (and Links to their respective Upstream Issues). Do the same in apps/APPS_INFO for any bundled libs in apps (that are shipped with core).

Then we also need to force this upon all apps the can be installed from the appstore, no?

@MorrisJobke
Copy link
Member

cc @rullzer @blizzz @schiessle @nickvergessen Opinions?

@MorrisJobke
Copy link
Member

The full list of JS dependencies should be https://github.com/nextcloud/server/blob/master/bower.json and the full list of PHP dependencies should be https://github.com/nextcloud/3rdparty/blob/master/composer.json

@LorbusChris
Copy link
Contributor Author

LorbusChris commented Feb 27, 2018

Unfortunately the mentioned composer/bower files are not complete:

The PHP composer.json lacks declaration of bundled deps of deps (see below, e.g. DBAL is bundled but provides another 6 bundled (php-doctrine) libs).

For JS, some bower.json files are not complete, and some apps lack such file altogether or they have been removed at some point (e.g. see this commit on the comments app). The same goes for non-existent LICENSE information on some JS libs (also comments app)

That is not possible with our current code to load JS files into templates, where we need t load it from js/...

Not all JS deps are in a js dir (e.g. user_ldap app).

Then we also need to force this upon all apps the can be installed from the appstore, no?

That would seem preferable to me, yes.

During dependency analysis I came up with this list (taken from my rpm spec). Note that for the PHP libraries, I only looked at the deps of the bundled libs that weren't in the Fedora repos, so I had to bundle them in the rpm (interersting here DBAL and sabre/dav). There is a NOTE for each dep not listed in a bower.json:

# External PHP libs required by core
# https://github.com/nextcloud/3rdparty/blob/v13.0.0/composer.json
# "aws/aws-sdk-php": "^3.35"
# "bantu/ini-get-wrapper": "v1.0.1"
# "deepdiver1975/TarStreamer": "v0.1.0"
# Despite the different namespace this lives in the name is correct
# "doctrine/dbal": "dev-2.5.-pg10"
# NOTE: NC13: Bundling patched DBAL
# NOTE: License: MIT
Provides:       bundled(php-doctrine-dbal) = 2.5.5
Provides:       bundled(php-doctrine-annotations) = 1.2.7
Provides:       bundled(php-doctrine-cache) = 1.5.1
Provides:       bundled(php-doctrine-collections) = 1.3.0
Provides:       bundled(php-doctrine-common) = 2.7
Provides:       bundled(php-doctrine-inflector) = 1.1.0
Provides:       bundled(php-doctrine-lexer) = 1.0.1
# "guzzlehttp/guzzle": "~5.3"
# "icewind/searchdav": "0.3.1"
# "icewind/Streams": "0.5.2"
# "interfasys/lognormalizer": "^v1.0"
# "jeremeamia/superclosure": "2.1.0"
# "leafo/scssphp": "^0.7.2"
# "league/flysystem": "^1.0"
# "lukasreschke/id3parser": "^0.0.1"
# "mcnetic/zipstreamer": "^1.0"
# NOTE: We use v1.1.x from DeepDiver1975's maintained fork that contains all required patches.
# "natxet/CssMin": "dev-master"
# "nikic/php-parser": "1.4.1"
# "patchwork/jsqueeze": "^2.0"
# "patchwork/utf8": "1.2.6"
# "pear/archive_tar": "1.4.3"
# archive_tar is in base el7 and doesn't have the fedora php-composer provides
# "pear/pear-core-minimal": "v1.10"
# "phpseclib/phpseclib": "2.0.4"
# "pimple/pimple": "3.2.3"
# "punic/punic": "^1.6"
# "rackspace/php-opencloud": "v1.16.0"
# "sabre/dav" : "^3.2.0"
# NOTE: Bundling patched sabre/dav
# NOTE: License: BSD-3-Clause
Provides:       bundled(php-sabre-dav) = 3.2.0
Provides:       bundled(php-sabre-event) = 3.0.0
Provides:       bundled(php-sabre-http) = 4.3.2
Provides:       bundled(php-sabre-uri) = 1.2.1
Provides:       bundled(php-sabre-vobject4) = 4.0
Provides:       bundled(php-sabre-xml) = 1.5.0
# "stecman/symfony-console-completion": "^0.7.0"
# "swiftmailer/swiftmailer": "^5.4"
# "symfony/console": "^3.3.0"
# "symfony/event-dispatcher": "^3.3.0"
# "symfony/polyfill": "^1.0"
# "symfony/process": "^3.3.0"
# "symfony/routing": "^3.3.0"
# "symfony/translation": "^3.3.0"

# External PHP libs required by files_external app
# https://github.com/nextcloud/server/blob/v13.0.0/apps/files_external/3rdparty/composer.json
# NOTE: icewind/streams is a dep but is already required by core
# "icewind/smb": "2.0.4"

# External PHP libs required by gallery app
# https://github.com/nextcloud/gallery/blob/v13.0.0/composer.json
# "symfony/yaml": "~3.2"

# bundled js libs in core/vendor
# https://github.com/nextcloud/server/blob/v13.0.0/bower.json
# autosize: MIT
Provides: bundled(js-autosize) = 3.0.17
# backbone: MIT
Provides: bundled(js-backbone) = 1.2.3
# base64: ASL 2.0
Provides: bundled(js-Base64.js) = 0.3.0
# blueimp-md5: MIT
Provides: bundled(js-blueimp-md5) = 2.7.0
# bootstrap: MIT
Provides: bundled(js-bootstrap) = 3.3.6
# clipboard: MIT
Provides: bundled(js-clipboard) = 1.5.12
# NOTE: DOMPurify is not in bower.json
# DOMPurify: MPLv2.0
Provides: bundled(js-DOMPurify) = 1.0.0
# davclient.js: MIT
Provides: bundled(js-davclient.js) = 0.1.2
# es6-promise: MIT
Provides: bundled(js-es6-promise) = 2.3.0
# handlebars: MIT
Provides: bundled(js-handlebars) = 4.0.5
# jcrop: MIT
Provides: bundled(js-jcrop) = 0.9.12
# jquery: MIT
Provides: bundled(js-jquery) = 2.2.0
# jquery-migrate: MIT
Provides: bundled(js-jquery-migrate) = 1.4.0
# jquery-ui: MIT
Provides: bundled(js-jquery-ui) = 1.10.0
# jsTimezoneDetect: MIT
Provides: bundled(js-jsTimezoneDetect) = 1.0.6
# marked: MIT
Provides: bundled(js-marked) = 0.3.6
# moment: MIT
Provides: bundled(js-moment) = 2.15.0
# select2: MIT
Provides: bundled(js-select2) = 3.4.8
# snapjs: MIT
Provides: bundled(js-snapjs) = 2.0.0-rc1
# strengthify: MIT
Provides: bundled(js-strengthify) = 0.5.3
# underscore: MIT
Provides: bundled(js-underscore) = 1.8.0
# zxcvbn: MIT
Provides: bundled(js-zxcvbn) = 4.4.2

# js libs bundled in apps/comments/js/vendor
# NOTE: no bower.json
# At.js: MIT
Provides: bundled(js-At.js) = 1.5.4
# Caret.js: MIT
Provides: bundled(js-Caret.js) = 0.2.2

# js libs bundled in apps/files_pdf_viewer/vendor
# NOTE: no bower.json
# pdfjs: ASL 2.0
Provides: bundled(js-pdfjs) = 1.4.20

# js libs bundled in apps/files_texteditor/js/core/vendor
# https://github.com/nextcloud/files_texteditor/blob/v13.0.0/js/bower.json
# ace-builds: BSD
Provides: bundled(js-ace-builds) = 1.2.8

# js libs bundled in apps/gallery/js/vendor
# https://github.com/nextcloud/gallery/blob/v13.0.0/js/bower.json
# NOTE: bigshot is not in bower.json
# bigshot: ASL 2.0
Provides: bundled(js-bigshot) = 0.27.0
# commonmark: BSD-2-Clause
Provides: bundled(js-commonmark) = 0.27.0
# NOTE: DOMPurify is bundled in core as well
# DOMPurify: MPLv2.0
Provides: bundled(js-dompurify) = 0.8.6
# eventsource-polyfill: MIT
Provides: bundled(js-eventsource-polyfill) = 0.9.7
# github-markdown-css: MIT
Provides: bundled(js-github-markdown-css) = 0.1
# NOTE:  jquery-touchevents and jqueryui-touch-punch are not in bower.json
# jquery-touch-events: MIT
Provides: bundled(js-jquery-touch-events) = 1.0.8
# jqueryui-touch-punch: MIT
Provides: bundled(js-jqueryui-touch-punch) = 0.2.3

# js libs bundled in apps/theming/js/3rdparty
# NOTE: no bower.json
# jscolor: GPLv3
Provides: bundled(js-jscolor) = 2.0.4

# js libs bundled in apps/user_ldap/vendor
# NOTE: no bower.json file
# jquery-multiselect: MIT
Provides: bundled(js-jquery-multiselect) = 1.13

I would appreciate a review of this list.
As I said, I'd be willing to PR this list (nicely formatted) to replace LICENSE INFO/patches.txt and update other files in the respective places.

Also note that I only looke at vendored libs that come with the main deliverable (the one that includes core and the "standard" apps, including gallery), but not at any other apps that need to be downloaded from the app store.

Edit: The list of Licenses I came up with is:
License: AGPLv3+ and ASL 2.0 and BSD and GPLv3+ and MIT and MPLv2.0

@MorrisJobke
Copy link
Member

Do you then bundle the exact same version of the libs? And also be aware that we put some patches in those libs: https://github.com/nextcloud/3rdparty/blob/master/patches.txt

I hope this is then okay?

@LorbusChris
Copy link
Contributor Author

LorbusChris commented Feb 28, 2018

For Fedora, I only bundle the js libs and the patched php libs (excluding the patched Zipstreamer lib, which is packaged seperately).

All other PHP libs are in the Fedora distro in their required version. This is not necessarily the exact version that is bundled here, see:
https://github.com/LorbusChris/nextcloud-rpm/blob/13.0.0/nextcloud.spec#L50
It should be compatible.

@MorrisJobke
Copy link
Member

It should be compatible.

We only guarantee that it works with the exact versions. Often we have workarounds in place for specific versions because the lib hasn't released a new version. So this could break the overall system in some cases. :/

I really appreciate your work, but we had people from distributions in the past, that did exactly this (especially Adam from Fedora IIRC) and then gave up, because building a web app is a lot harder then the usual libs and applications on an OS and the neverending update cycles with updated dependency versions and stuff like that are simply not suited to be taken out of the release we do. I don't want to de-motivate you, but rather prepare you to this hard endeavor. Using just our tar ball and distribute that in a package is the safer approach and also what we test extensively. Also this would allow to track bugs here and not do this on your side as well. Because if you bundle Nextcloud with different versions of the lib we can not guarantee anything and the users then need to test again with our version. Also keep in mind, that an update typically also involve a maintenance step (calling occ upgrade) and that you can only upgrade from 11 to 12 and from 12 to 13 (and skipping versions is not allowed).

@LorbusChris
Copy link
Contributor Author

@MorrisJobke thanks for the heads up.
In the Fedora RPM, any lib that is indeed patched, is being bundled as-is.
Only unpatched php deps with a compatible version in the Fedora repos will not be bundled.
Also, all of the JS libs are bundled without change.

NC13 is my entry point to this story. I will make sure there's an upgrade path from here on out (>13). Personally, I will not support upgrading from any version below it, as I've never used them. (PRs on the RPM are of course welcome!)

@MorrisJobke
Copy link
Member

Maybe also have a look at the ownCloud packages: https://build.opensuse.org/project/show/isv:ownCloud:community

They maybe give you some hint on how it was done. They were pretty stable - not perfect, but worked. Maybe it helps you. The Nextcloud project just dropped the work on this, because it was a huge amount of time that went into this with still some rough parts. So we decided to only provide the tar balls and a built-in updater.

But back to the original point: We would be happy if you update the license list in the repos. @schiessle What is your opinion here, do we need this from a legal point? (I don't think so, because the license is listed for every dependency on it's own anyways) And should we somehow automatically compile this list from composer? cc @rullzer

@LorbusChris
Copy link
Contributor Author

Unfortunately, not all deps come with their license info (see, e.g. At.js and Caret.js in comments app).
Automatic gathering of all included Licenses would be a huge plus!
I'll wait a few more days for input fom your side and will then PR the list :)

@LorbusChris
Copy link
Contributor Author

Another thought:
It would be easy to create a second RPM for Fedora that doesn't unbundle anything.
This RPM, while not eligible to live in the official Fedora distro repos, would then obviously ship with the exact, well tested dep versions from this repo.
(I really do want to get NextCloud >=13 into Fedora proper, but I wouldn't mind creating this second RPM for anyone who doesn't care about distros/shared libs and e.g. just wants to run NC on a Fedora Container)

@LorbusChris
Copy link
Contributor Author

LorbusChris commented Mar 22, 2018

My new proposal (see PRs referenced above) includes suggestion of a few rules for future app development:

  • PHP deps go in vendor/
  • JS deps go in js/vendor/
  • each app that lives in its own repo and that has deps needs to have a DEPENDENCY_INFO.md file
  • it is encouraged it to use and keep up to date composer.json and bower.json files with exact versions specified for dep management

@LorbusChris
Copy link
Contributor Author

LorbusChris commented Apr 3, 2018

@MorrisJobke

We only guarantee that it works with the exact versions.

I think this should be reflected in the bower/composer files by removing the ^ and ~ before the versions in order to specify an exact version. Thoughts?

@MorrisJobke
Copy link
Member

I think this should be reflected in the bower/composer files by removing the ^ and ~ before the versions in order to specify an exact version. Thoughts?

Makes a lot of sense to pin the exact version. cc @icewind1991 @rullzer @schiessle @blizzz @nickvergessen

@icewind1991
Copy link
Member

Pinning the versions is good.

I'm not a big fan of the DEPENDENCY_INFO.md files, I can see the use of them but 95% of the time all info can be gottom from the package.json/composer.json files and a separate file is bound to get out of sync sometimes.

@LorbusChris
Copy link
Contributor Author

The files are supposed to also include info about patches, gathering all dep info in one place.

However, if all package/bower/composer files were in fact kept up to date (or even existed), maybe we wouldn't need them.

@LorbusChris
Copy link
Contributor Author

LorbusChris commented Apr 23, 2018

@MorrisJobke @rullzer @blizzz @schiessle @nickvergessen
are we moving forward with this? I'd go ahead and rebase/fix naming in all submitted PRs then...

@nextcloud-bot nextcloud-bot added the stale Ticket or PR with no recent activity label Jun 20, 2018
@jospoortvliet
Copy link
Member

@LorbusChris any chance you'd be at the Nextcloud conf? A face to face would make this conversation easier, perhaps ;-)

@nextcloud-bot nextcloud-bot removed the stale Ticket or PR with no recent activity label Aug 16, 2018
@LorbusChris
Copy link
Contributor Author

@jospoortvliet I'll be there Thu - Sun!

@MorrisJobke
Copy link
Member

As we discussed this at the conference it's a tricky topic. I would like to not do it for now as it basically adds a lot of maintenance work for not much outcome. Sorry for this.

I hope that's okay for you.

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

No branches or pull requests

5 participants