Server: Server upgrade not working without viewer

[juliushaertl]

• Install server from git for development purposes
• Have changes that bump the version or manually bump ìt in version.php

• Viewer is not installed but we get:

  The files of the app (viewer) were not replaced correctly. Make sure it is a version compatible with the server.

I think it should still be possible to run the server in dev setups without viewer being installed. cc @skjnldsv @rullzer

Caused by https://github.com/nextcloud/server/pull/18334 as always enabled apps are considered being installed no matter if they actually are https://github.com/nextcloud/server/blob/3eee359d7ffd6219f6a2ab8cca18a13118552842/lib/private/App/AppManager.php#L137

[rullzer]

Yeah I know. But without viewer a lot of things do boom.
Maybe we just add a dev note to the readme to always install the viewer as well.

[skjnldsv]

Yeah, no idea what to do here.
It's pretty much the same with lots of apps for me.
Support, password policy, files_texteditor, firstrunwizard, notifications... Every update it's the same :p

[juliushaertl]

Those are different, because the server works fine if they are not installed at all (they are only marked as shipped), but viewer is the only one that is always enabled but not part of the repo.

Anyway I'm also fine with just properly documenting that. It just adds an additional step when getting started.

[skjnldsv]

Ah right! Server should work without viewer. We just emit some Viewer events, but they're checked before emit iirc, so we should do that sure :)

[tonyguepin]

This still is an issue in v18.0.6.
I guess that, when viewer is not taken out of the core/shipped.json alwaysEnabled list,
then the app should be part of the standard nextcloud layout.

[landryb]

occ upgrade always break when using a checkout from git, so one way or another (submodule? import viewer code?) it should be fixed..

[juliushaertl]

git clone https://github.com/nextcloud/viewer.git into your apps directory

[landryb]

well, sure now that im aware of this issue and understood the cryptic message that lead me there, but it isnt obvious for other users ;)

[PhrozenByte]

Ah right! Server should work without viewer. We just emit some Viewer events, but they're checked before emit iirc, so we should do that sure :)

Do I understand you right @skjnldsv that we can indeed remove viewer from alwaysEnabled in shipped.json?

By the way, I didn't experience any issues with the latest Git version until a upgrade was triggered by one of my dev apps. It looks like this is no huge issue... viewer is in shipped.json's alwaysEnabled since 17.0.5RC1 and was introduced by @rullzer (https://github.com/nextcloud/server/commit/eb8ca3783c49ad432338735b47d17dba46ed3f7b). @rullzer do you remember the reasons for this?

This still is an issue in v18.0.6.
I guess that, when viewer is not taken out of the core/shipped.json alwaysEnabled list,
then the app should be part of the standard nextcloud layout.

I agree. If Nextcloud server relies on viewer, its code should be moved to the server repo. Otherwise it breaks dev installations - and I just wasted about half an hour of valuable time just to fix this. As an alternative we could remove or refactor any code depending on viewer. That's probably the better solution. Where do we rely on viewer?

[skjnldsv]

Do I understand you right @skjnldsv that we can indeed remove viewer from alwaysEnabled in shipped.json?

No, Viewer needs to stay enabled.
It is required for other apps to work, because we currently don't support cross-apps dependencies
https://github.com/nextcloud/server/pull/18334

[PhrozenByte]

It is required for other apps to work, because we currently don't support cross-apps dependencies

What does this exactly mean? #18334 unfortunately doesn't give any reasoning.

Is this about events only? Because #18334 references https://github.com/nextcloud/photos/issues/38 and the actual event class isn't required to listen on events. Simply replace LoadViewer::class by "OCA\Viewer\Event\LoadViewer". If you really need the implementation (e.g. because you're dispatching the event), provide a custom implementation by wrapping a class_exists() clause around a provided copy of the class.

Please also note that it isn't quite right that you can't check for dependencies before enabling a app. Simply add the checks you require to a postSchemaChange in a migration step and throw a exception with a nice error message if the requirements aren't met. This error message is shown to the user. I do this for cms_pico because Pico CMS for Nextcloud must have permission to write to certain folders, see Migration\Version010000::postSchemaChange() and Service\MiscService::checkPublicFolder(). You can do the same when checking whether nextcloud/viewer is enabled or not.

[skjnldsv]

@PhrozenByte migrations are only executed once.
So as far as I know, having a check here would work for the first time the migrations ran.
This would not stop anyone from enabling an app that require viewer to work. :/

This is not about php events, a php class check will do the trick.
This is about features. Enabling the files_pdfviewer without viewer doesn't make any sense and the admin/user needs to be aware of it. :shrug:

PR welcome! Unfortunately that is a substantiate amount of work. In the meantime, just enable viewer and you'll be good to go. It's not that much of a crazy step !
Have a great day! Cheers

[PhrozenByte]

@PhrozenByte migrations are only executed once.
So as far as I know, having a check here would work for the first time the migrations ran.
This would not stop anyone from enabling an app that require viewer to work. :/

Good point. Didn't remember my own implementation right, that's the reason why the check is also performed within a repair step - which is indeed run on every upgrade. See Migration/AppDataRepairStep::run().

This is not about php events, a php class check will do the trick.
This is about features. Enabling the files_pdfviewer without viewer doesn't make any sense and the admin/user needs to be aware of it. :shrug:

This is another optional app. If files_pdfviewer depends on viewer, files_pdfviewer should check for it - or document it if it's not a technical dependency, but a usage dependency. Adding viewer to alwaysEnabled simply is the wrong approach. Since when is adding custom app logic to server okay? I'd do this all the time then, makes a lot of stuff easier. But we could then probably throw away the apps concept in general and return to having everything within server.

PR welcome! Unfortunately that is a substantiate amount of work. In the meantime, just enable viewer and you'll be good to go. It's not that much of a crazy step !

Don't get me wrong, but breaking installations just because optional apps depend on other optional apps seems a bit excessive. The problem is that this is causing issues for everybody who uses a Git-based installation of Nextcloud - not just for devs. It wastes time every single time one sets up a Git-based installation. If it's just 15 minutes each you end up wasting 250 hours of work per a thousand installations. nextcloud/server is cloned by >250 unique users per day. Just because it's not your time it still is time wasted :wink:

If viewer is that important it should be moved to the server repo. This is no "substantiate amount of work".

[skjnldsv]

The problem is that this is causing issues for everybody who uses a Git-based installation of Nextcloud

Yeah, don't ever do that :)

Adding viewer to alwaysEnabled simply is the wrong approach

Don't get me wrong, the correct approach would indeed be to have a proper dependency management. For now, this is good enough and works as intended.

This is another _optional_ app.

You're mistaking optional and shipped. If this is shipped, viewer needs to be installed and enabled by default.

I'll stop arguing as this is going nowhere. This is how it is. You're a dev, there are plenty of other steps that are annoying (cloning submodules... etc), cloning viewer is one of them.
Unless someone does the work to create a proper app dependency mechanism, we will not change anything here.

---

If it's just 15 minutes each you end up wasting 250 hours of work per a thousand installations. nextcloud/server is cloned by >250 unique users _per day_

Yeah, 99.99% of those are our CI testing, I can pretty much guarantee that.
And cloning viewer is a 10 second step. Let's not exaggerate, thanks.

[skjnldsv]

Locked, no point in creating more noise, the issue is opened, we all want that, we all agree. Viewer needs to be enabled for shipped apps.
Please open pr if you have a nice solution that cover all the use cases! :rocket: