set defaultEnabled in shipped.json

[ArtificialOwl]

The main idea is to remove the non-official tag <default_enable/> from apps' info.xml so it is possible to release version on the appstore of apps distributed with core.

• add array defaultEnabled in core/shipped.json
• add method in IAppManager: isDefaultEnabled(), getDefaultEnabledApps
• add check during installation process in Installer.php
• keep old/deprecated check on <default_enable/> for retro-compatibility in Installer.php
• remove deprecated <default_enable/> from apps available in this repo

Todo

• Developer documentation for the new API

[ArtificialOwl]

not sure the fail check is related

[come-nc]

Apart from small question, looks good.
Is it necessary to keep support for the xml tag? We know the exact list of default enabled apps, no?

[skjnldsv]

Is it necessary to keep support for the xml tag? We know the exact list of default enabled apps, no?

Same question, better remove that now?

[ArtificialOwl]

Is it necessary to keep support for the xml tag? We know the exact list of default enabled apps, no?

Same question, better remove that now?

just to be sure that no app is forgotten in next build

[ArtificialOwl]

@nickvergessen : list of apps should now be complete, it is based on a clear setup of beta6 and a grep in apps/ on 'default_enable'

@skjnldsv @come-nc : removing retro-compatibility and its tech debt as now the list of apps should be complete

[skjnldsv]

Restarted CI, as it seems to be failing for another reason.

[skjnldsv]

9 errors, seems relevant

1) OCA\UpdateNotification\Tests\Settings\AdminTest::testGetFormWithUpdate
244 | ArgumentCountError: Too few arguments to function OCA\UpdateNotification\Settings\Admin::__construct(), 8 passed in /drone/src/apps/updatenotification/tests/Settings/AdminTest.php on line 90 and exactly 9 expected
245 |  
246 | /drone/src/apps/updatenotification/lib/Settings/Admin.php:58
247 | /drone/src/apps/updatenotification/tests/Settings/AdminTest.php:90
248 |  
249 | 2) OCA\UpdateNotification\Tests\Settings\AdminTest::testGetFormWithUpdateAndChangedUpdateServer

[skjnldsv]

Ah, maybe not, I see other PRs with the same error

[PVince81]

in other news, seems the apps acceptance tests also fail, likely related

[ArtificialOwl]

documentation does not seems necessary

[summersab]

Is it necessary to keep support for the xml tag? We know the exact list of default enabled apps, no?

Same question, better remove that now?

@ArtificialOwl @come-nc @skjnldsv Was there a good reason for removing the "old/deprecated check on <default_enable/> for retro-compatibility in Installer.php?" I have several custom apps that I marked as <default_enable/> so that they would be enabled by default when a I deploy and install a new image. Some of them execute code during the installation process and before the default admin is created. Now, that doesn't happen, and I don't see a good way to achieve the same behavior. I COULD change core/shipped.json, but then, my server would fail integrity checks.

Previously, $shippedApps and $defaultEnabled were mutually exclusive lists. Now, $defaultEnabled is a subset of and dependent upon $shippedApps - there is no way to identify apps that are not shipped but should be enabled. Furthermore, this change makes the list of default apps hard-coded instead of dynamic. As we all know, hard-coding things in code is generally avoided unless there's a good reason.

In my opinion, the original mechanism should have been left intact even if a new method is being used to mark shipped apps as enabled by default. If you have an alternative method that I could use (or a more efficient mechanism that could be added instead), that would work for me. While I may be one of the only people who took advantage of this feature, removing it has made my deployments difficult.

[come-nc]

@ArtificialOwl @come-nc @skjnldsv Was there a good reason for removing the "old/deprecated check on <default_enable/> for retro-compatibility in Installer.php?" I have several custom apps that I marked as <default_enable/> so that they would be enabled by default when a I deploy and install a new image. Some of them execute code during the installation process and before the default admin is created. Now, that doesn't happen, and I don't see a good way to achieve the same behavior. I COULD change core/shipped.json, but then, my server would fail integrity checks.

Previously, $shippedApps and $defaultEnabled were mutually exclusive lists. Now, $defaultEnabled is a subset of and dependent upon $shippedApps - there is no way to identify apps that are not shipped but should be enabled. Furthermore, this change makes the list of default apps hard-coded instead of dynamic. As we all know, hard-coding things in code is generally avoided unless there's a good reason.

In my opinion, the original mechanism should have been left intact even if a new method is being used to mark shipped apps as enabled by default. If you have an alternative method that I could use (or a more efficient mechanism that could be added instead), that would work for me. While I may be one of the only people who took advantage of this feature, removing it has made my deployments difficult.

I would say the default enabled mechanism was never intended to be used like you did.
Can you clarify why your applications cannot use a migration for their installation like any other application?

[summersab]

@come-nc My applications do use migrations for their installation - that isn't the problem. I want these custom apps to be installed and enabled as part of the NC installation process. More simply, if I run php occ maintenance:install, my custom applications should be installed and enabled. Using the <default_enable/> tag made this possible.

[come-nc]

@come-nc My applications do use migrations for their installation - that isn't the problem.

I’m pretty sure that is the problem. How are they running code in the installation process without a migration?

[summersab]

I believe we're talking about two different installation processes. I think you're talking about the application's installation process. I'm referring to the installation of Nextcloud itself - the initial first install. That is when I want my applications to be installed and enabled. My applications install just fine once a Nextcloud instance is up and running. I want my custom applications to be installed and enabled during the first install via OC\Installer::InstallShippedApps:

server/lib/private/Installer.php

Line 554 in 069477e

Installer::installShippedApp($filename);

[nickvergessen]

But that is not intended. "Install shipped app" is for installing shipped apps, which is the apps we ship in the official zip/tar file.
Anything else you have to install with a follow up command like occ app:enable my-private-app and done.

[summersab]

But that is not intended. "Install shipped app" is for installing shipped apps, which is the apps we ship in the official zip/tar file.
Anything else you have to install with a follow up command like occ app:enable my-private-app and done.

Now you're talking about shipped apps. I'm talking about default enabled apps. They are different (or, at least, they were).

Previously, these were mutually exclusive categories. Shipped apps were not always default enabled, and not all default enabled apps had to be shipped - this may not have been intended, but it was indeed a feature and a capability that has been removed. Now, default enabled apps are a subcategory of and dependent upon the list of shipped apps.

Also, the default apps have become a hard-coded static list with this change. Previously, I could mark any app I wanted as <default_enable>, and the installer took care of the rest. Now, not only can I not do that, but I have to hard-code my own static list of occ app:enable my-private-app commands.

Another reason why this is problematic is because it makes bundling my own "distribution" of Nextcloud difficult. From an open source standpoint, it was easy to bundle my own custom apps that get enabled by default and redistribute this as a "flavor" of Nextcloud. Now, I can't do that. I either have to modify shipped.json (which breaks code signing) or instruct users to manually run occ app:enable my-private-app to take advantage of my distribution.

Imagine if Ubuntu was simply Debian with a bunch of custom *.deb files shoved in the /opt directory, and for some reason, Debian makes it impossible for those packages to be installed by default. Instead, users have to install the OS and THEN know to manually install those packages with dpkg in order to convert their systems into Ubuntu. That would defeat the whole point of making a system that is open, easy to modify, and able to be redistributed. From my position, it is essentially what has been done here.

I'm sorry for making a big deal out of this. I may be the only developer who took advantage of this feature, but I really did rely on it (whether it was intended or not). Is there any performance reason why the legacy code couldn't be added back into the source? It doesbt seem to cause any harm (and from my point of view, it makes the system more malleable, which I would argue is a good thing).

[nickvergessen]

Previously, these were mutually exclusive categories

No, default enabled apps were always designed to be a subset of shipped apps.
You can easily confirm this by comparing the XSD files used to validate apps:

• https://github.com/nextcloud/server/blob/master/resources/app-info-shipped.xsd
• https://github.com/nextcloud/server/blob/master/resources/app-info.xsd

Also apps with the <default_enable> element are not uploadable on the appstore, as they don't validate with the second XSD file.

it was easy to bundle my own custom apps that get enabled by default and redistribute this as a "flavor" of Nextcloud.

That sounds like a perfect case for the Bundles that we have:
https://github.com/nextcloud/server/tree/master/lib/private/App/AppStore/Bundles
Not sure if that would be extendable into a direction like this, but also then it depends on the appstore and you have to upload your custom flavor apps to the appstore first.

Is there any performance reason why the legacy code couldn't be added back into the source? It doesbt seem to cause any harm (and from my point of view, it makes the system more malleable, which I would argue is a good thing).

Looking at the code it's mostly about readability of the code and since it was never meant to be working like that in first place the old way was replaced.

[summersab]

Previously, these were mutually exclusive categories

No, default enabled apps were always designed to be a subset of shipped apps.

Perhaps that is the way it was designed, but in practice, they were mutually exclusive, and it did provide this functionality. Also, just because a feature isn't intentional doesn't mean it is invalid or not useful. There are plenty of unintentional features in IT that wound up becoming extremely useful:

• IP addresses were supposed to be randomly assigned, not fixed to geographical regions. We now use this for basic geolocation across the entire IT industry.
• CATV coax cable was designed for distributing TV signals. RJ11/14 was designed for telephone service. Both carried analog signals. Neither were intended to be used for distributing digital internet signals.
• The Raspberry Pi was launched with the intention of being an educational device. It is now used in production by thousands of companies for industrial control, digital displays, web hosting, etc. NASA even uses them.

it was easy to bundle my own custom apps that get enabled by default and redistribute this as a "flavor" of Nextcloud.

That sounds like a perfect case for the Bundles that we have: https://github.com/nextcloud/server/tree/master/lib/private/App/AppStore/Bundles Not sure if that would be extendable into a direction like this, but also then it depends on the appstore and you have to upload your custom flavor apps to the appstore first.

Bundles don't provide the same functionality. There is still no way to have a custom app or bundle automatically installed during the standard installation process. I can't create my own *.zip distribution of Nextcloud that auto-installs and enables my custom apps like I could with the <default_enable/> tag.

I realize that I'm not going to be changing minds on this topic or convincing anyone to add the legacy code added back into the project. Instead, could we discuss how to develop this as an actual formal feature?

[come-nc]

• IP addresses were supposed to be randomly assigned, not fixed to geographical regions. We now use this for basic geolocation across the entire IT industry.
• CATV coax cable was designed for distributing TV signals. RJ11/14 was designed for telephone service. Both carried analog signals. Neither were intended to be used for distributing digital internet signals.
• The Raspberry Pi was launched with the intention of being an educational device. It is now used in production by thousands of companies for industrial control, digital displays, web hosting, etc. NASA even uses them.

You forgot Doom was never intended to be used as a process management user interface, and yet https://psdoom.sourceforge.net
For me this is a clear case of https://xkcd.com/1172

Bundles don't provide the same functionality. There is still no way to have a custom app or bundle automatically installed during the standard installation process. I can't create my own *.zip distribution of Nextcloud that auto-installs and enables my custom apps like I could with the <default_enable/> tag.

I realize that I'm not going to be changing minds on this topic or convincing anyone to add the legacy code added back into the project. Instead, could we discuss how to develop this as an actual formal feature?

If you are bundling Nextcloud and distributing it as zip, you can edit the defaultEnabled array in core/shipped.json. You can even remove applications from the list, which you could not do without modifying the application before.

From what you said your main blocker for that is the code integrity check, which I am not super familiar with, but maybe you can update the signature and use your own cert? The code will not validate with Nextcloud root certificate, but from what I understand that is the point of the system, that third party distributing forks cannot pass them as our official version, or something like that.

[szaimen]

Or maybe simply disabled the integrity check?