YunoHost-Apps / hotspot_ynh

Wifi Hotspot app for YunoHost
GNU Affero General Public License v3.0
39 stars 19 forks source link

Buster armbian enhancements #71

Closed HugoPoi closed 3 years ago

HugoPoi commented 3 years ago

Problem

Solution

For non-free support on Armbian

Hot reload driver module

PR Status

Package_check results


If you have access to App Continuous Integration for packagers you can provide a link to the package_check results like below, replacing '-NUM-' in this link by the PR number and USERNAME by your username on the ci-apps-dev. Or you provide a screenshot or a pastebin of the results

Build Status

HugoPoi commented 3 years ago

And I have tried my change for ynh_install_extra_app_dependencies, I need to check why.

HugoPoi commented 3 years ago

I have tested the upgrade

admin@lime2-test:~$ sudo yunohost app upgrade hotspot -u https://github.com/HugoPoi/hotspot_ynh/tree/fix/buster-armbian-enhancements --force
Info: Now upgrading hotspot...
Info: [+...................] > Loading installation settings...
Info: [#+..................] > Checking version...
Info: [##+.................] > Ensuring downward compatibility...
Info: [###+................] > Backing up the app before upgrading (may take a while)...
Info: [####+...............] > Stopping a systemd service...
Info: [#####+..............] > Upgrading source files...
Info: [######+.............] > Upgrading nginx web server configuration...
Info: [#######+............] > Upgrading dependencies...
Info: [########++..........] > Making sure dedicated system user exists...
Info: [##########+.........] > Upgrading php-fpm configuration...
Info: [###########+........] > Copying configuration...
Info: [############+.......] > Modifying a config file...
Warning: File /var/www/hotspot/config.php has been manually modified since the installation or last upgrade. So it has been duplicated in /home/yunohost.conf/backup//var/www/hotspot/config.php.backup.20210321.165915
Warning: --- /home/yunohost.conf/backup//var/www/hotspot/config.php.backup.20210321.165915  2021-03-21 16:57:04.151566464 +0000
Warning: +++ /var/www/hotspot/config.php    2021-03-21 16:59:15.954007615 +0000
Warning: @@ -22,11 +22,11 @@
Warning:  function configure() {
Warning:    option('env', ENV_PRODUCTION);
Warning:    option('debug', false);
Warning: -  option('base_uri', '__PATH__/');
Warning: +  option('base_uri', '/wifiadmin/');
Warning:    layout('layout.html.php');
Warning: -  define('PUBLIC_DIR', '__PATH__/public');
Warning: +  define('PUBLIC_DIR', '/wifiadmin/public');
Warning:  }
Warning:  // Before routing
Info: [#############+......] > Upgrading systemd configuration...
Info: [##############+.....] > Securing files and directories...
Info: [###############+....] > Integrating service in YunoHost...
Warning: Try to reload driver for usb 2-1
Info: [################+...] > Starting a systemd service...
Info: [#################+..] > Reloading nginx web server...
Info: [##################++] > Upgrade of hotspot completed
Success! hotspot upgraded
Success! Upgrade complete

I suspect a existing bug minor bug with /var/www/hotspot/config.php (not sure because all working ok)

alexAubin commented 3 years ago

Hmpf yeah that's not a super huge issue

This is related to the way we check that some config files were edited or not ... which in fact was never really properly thought / designed what we aim to do I believe ...

In particular, here the check/backup happens here : https://github.com/labriqueinternet/hotspot_ynh/blob/01c5115b2054de02fa96d39892b59f34b99439a6/scripts/upgrade#L172

And it's finding that indeed the file was modified manually because we savagely replace all files here: https://github.com/labriqueinternet/hotspot_ynh/blob/01c5115b2054de02fa96d39892b59f34b99439a6/scripts/upgrade#L108

Naively I would move the ynh_backup_if_checksum_is_different before the cp ...

Starting with 4.1, the good practice is rather to have a config template in conf/ and then call ynh_add_config --template="foo.conf.tpl" --destination="$final_dir/foo.conf" which will handle all the store / backup-if-checksum-different for you ... though that doesn't solve the issue that the cp will overwrite the conf so hmpf. Some people developed some unofficial mechanism to replace all-files-except-a-few-ones ...

HugoPoi commented 3 years ago

@alexAubin my last commit address your points, no more warnings ! I have tested the upgrade from the current version :-D

alexAubin commented 3 years ago

Alrighty let's merge it !