fix(docker): install the composer dependencies of cloned shipped apps - #1064
Conversation
| */ | ||
| verbose: boolean | ||
| /** | ||
| * Working directory to run the command in. Defaults to the container's working directory. |
There was a problem hiding this comment.
| * Working directory to run the command in. Defaults to the container's working directory. | |
| * Working directory to run the command in. Defaults to the Nextcloud root. |
There was a problem hiding this comment.
(or similar - as otherwise you would need to know the container structure)
There was a problem hiding this comment.
Applied, thanks. It's also more accurate than what I had, since runExec(['cat', 'core/shipped.json']) and runOcc already rely on that default.
| async function completeClonedApp(app: string, container: Container) { | ||
| const appPath = `/var/www/html/apps-writable/${app}` | ||
| if (!await isComposerAutoloaderBroken(appPath, container)) { | ||
| return | ||
| } | ||
|
|
||
| console.log(`│ ├─ ${app} was cloned without the Composer dependencies its autoloader needs`) | ||
| if (await installComposerDependencies(app, appPath, container)) { | ||
| return | ||
| } | ||
|
|
||
| await runExec(['rm', '-rf', `${appPath}/composer`], { container }) | ||
| console.log(`│ ├─ Removed '${app}/composer' to make the app loadable without its Composer autoloader`) | ||
| console.log(`│ └─ ⚠️ ${app} is incomplete, code using its Composer dependencies (e.g. 'lib/Vendor') will fail`) | ||
| } |
There was a problem hiding this comment.
Why not simply:
IF composer.json exists
THEN composer install --no-dev
?
There was a problem hiding this comment.
That was my first attempt, but it breaks on the two different layouts these apps use.
viewer and text have a second composer project in composer/composer.json with "vendor-dir": ".", so composer/ is their vendor dir and it's committed (composer/composer/autoload_real.php is right there in the repo). Their root composer.json is just dev tooling: viewer has an empty require, text only requires php.
notifications is the newer layout. Root composer.json is the runtime project, the vendor dir is the default vendor and git ignored, and composer/autoload.php is a hand written shim that requires it.
So "root composer.json exists, run install" would install the dev tooling project for viewer and text, while their actual runtime autoloader in composer/ is already fine. Nothing gets fixed, it just costs time.
The bigger problem is the combination with the rm -rf composer fallback. If that pointless install fails for any reason, we'd delete a committed, working vendor dir and break an app that was healthy. Running the autoloader only touches apps that would really make the server fatal, and it costs two docker exec round trips per cloned app.
Happy to go your way if you still prefer it. I'd just keep the fallback behind the autoloader check so a working composer/ can never get removed.
There was a problem hiding this comment.
So "root composer.json exists, run install" would install the dev tooling project for viewer and text, while their actual runtime autoloader in composer/ is already fine. Nothing gets fixed, it just costs time.
Why? I checked both cases and they do not have runtime dependencies.
So yes would waste time but a small amount only because with --no-dev nothing is installed.
The bigger problem is the combination with the rm -rf composer fallback.
Why would that be needed? I never had problem cloning any shipped app inside a Nextcloud checkout.
git clone + composer i --no-dev always works for all shipped apps I tried to clone (including text, notifications, activity, viewer, files_downloadlimit etc).
we'd delete a committed, working vendor dir
Which apps checks in vendor? This sound not really a good thing to do 👀
There was a problem hiding this comment.
You're right, switched to your version.
Also my wording was wrong: nothing checks in vendor. viewer and text commit composer/, which is the vendor dir of a separate composer project at composer/composer.json ("vendor-dir": "."). Their root vendor/ is git ignored like everywhere else.
I checked your point in the container, git clone + composer install --no-dev on viewer and text takes 0s, exits 0, and their committed composer/ autoloader still loads afterwards. So it is now simply: composer.json exists, run install. The autoloader probe and the fallback are gone, a failing install fails the setup.
97897f2 to
e25ec03
Compare
`app:list` was queried before `apps.config.php` registers `apps-writable` as an apps path, so a bind mounted app is missing from the list. It is then not found in `shipped.json` either and ends up in the app store branch, where `app:install --force` exits non-zero with "<app> already installed". Signed-off-by: Florian Ludwig <florian@krautnerds.de>
Shipped apps that are missing from the server image are installed with a plain
`git clone`, which only works as long as the app commits its composer
dependencies. Since Nextcloud 34 `nextcloud/notifications` no longer does, but
still ships `composer/autoload.php` requiring the git ignored
`vendor/autoload.php`. `OC_App::registerAutoloading()` requires that file, so
every request and every `occ` call dies with:
Failed opening required
'/var/www/html/apps/notifications/composer/../vendor/autoload.php'
#0 lib/private/legacy/OC_App.php(117): require_once()
nextcloud-libraries#1 lib/private/AppFramework/Bootstrap/Coordinator.php(78)
Run `composer install --no-dev` for a cloned app that has a `composer.json`.
Composer itself is not part of the server image, so it is downloaded into the
container once, `NEXTCLOUD_E2E_COMPOSER_VERSION` pins the version if needed.
Scripts are run on purpose, apps like notifications only assemble the prefixed
copies of their dependencies in `post-install-cmd`.
Fixes nextcloud-libraries#1059
Signed-off-by: Florian Ludwig <florian@krautnerds.de>
61a308a to
f11eee4
Compare
|
@susnux @nickvergessen updated the description here as well, it still described the previous revision. The implementation now is just: cloned app has a Since the approach changed rather than just got polished, the earlier approval probably does not carry over. Would appreciate another look. |
3 participants
Note
Stacked on #1063, so the first commit belongs to that PR. Please merge #1063 first, without it
configureNextcloudaborts before it ever reaches this code path when an app is bind mounted.Fixes #1059. Companion to nextcloud/notifications#3206, where @nickvergessen pointed at this spot in
lib/docker.tsas the place to solve it.Problem
Shipped apps that are missing from the CI server image are installed with a plain
git clone, which only works as long as the app commits its composer dependencies.Since
stable34,nextcloud/notificationsno longer bundles them, but still ships acomposer/autoload.phpdoingrequire_once __DIR__ . '/../vendor/autoload.php', andvendor/is git ignored.OC_App::registerAutoloading()requires that file, so every request and everyocccall dies:On
stable33thecomposer/directory did not exist yet, which is why this only surfaces when a suite moves tostable34. 0.4.0 and 0.5.0 are affected alike, the clone logic is unchanged between them.Fix
If a cloned app has a
composer.json, runcomposer install --no-devfor it. The server image ships PHP but no composer, socomposer.pharis downloaded into the container once.NEXTCLOUD_E2E_COMPOSER_VERSIONpins the version for consumers who want that, the default islatest-stable.Scripts are run on purpose. Apps like
notificationsonly assemble the prefixed copies of their dependencies (lib/Vendor/, viabamarni/composer-bin-pluginpluscoenjacobs/mozartinpost-install-cmd) there, so skipping scripts would leave the app loadable but still broken for web push.A failing install fails the setup, and the public API is unchanged.
An earlier revision of this PR probed the app's composer autoloader and, when the install failed, degraded to removing the cloned
composer/directory. Both are gone after @susnux's review, see the discussion below.Alternatives considered
composer installon the host, result shipped in viacontainer.putArchive(the workaround by @theCalcaholic in Missing bundled PHP dependencies (stable34) nextcloud/notifications#3206). Works, but adds a host dependency on PHP and composer. Given onubuntu-latest, not given for contributors running the suite locally. The in-container variant behaves the same on Linux runners and on macOS with Docker Desktop.composer/directory (my own workaround in Support installing composer packages for Nextcloud apps #1059). Needs no toolchain at all, butlib/Vendor/stays absent, so anything touching web push logsClass "OCA\Notifications\Vendor\Minishlink\WebPush\VAPID" not found.vendor/, and it would remove this code path entirely. Rejected because it gives up matching the app to the server branch, which is the reason the clone exists.continuous-integration-shallow-server. Until then this keeps consumers working without an image change.composer.pharagainst its published checksum. Left out: it is served from the same host over the same HTTPS connection, so it proves transfer integrity rather than authenticity, while the code deliberately executes third-party scripts from a freshgit clonemoments later.Testing
npm ci,npx tsc --noEmit,npm run lint,npm run buildclean (lint: 0 errors, the 8 warnings are pre-existing inlib/commands/*)npm run test:node4/4 pass, withnotificationsadded to the suite plus an assertion that itsvendor/autoload.phpexists and that the app is enabledRunning the install for every cloned app is cheap, measured in the server image after a
git clone --depth=1:composer install --no-devcomposer/End to end against a real
stable34server, usingnextcloud/attendance(the consumer from #1059) with this build linked in and its test server bumpedstable33tostable34:Completeness checked in the container, not just "it boots":
Not verified
stable34, only the server setup was exercised. Failures there would be NC33 to NC34 UI changes, unrelated to this.notificationsactually hitting the missing autoloader.