redesign installer logs #4539
redesign installer logs #4539
Conversation
There was a problem hiding this comment.
Nice work! It was on my low priority todo list 👍
With DSM7 all stdout during installation is written to the logfile by the system and output to stderror is presented to the user in a modal dialog.
From my testing it's the other way around. stderr is logged and stdout is presented to the user.
1>&2 redirects stdout to stderr
mk/spksrc.service.create_links
Outdated
| mkdir -p "$(dirname /usr/local/${cmd})" >> "${INST_LOG}" 2>&1 | ||
| echo "create link: /usr/local/${cmd} -> ${SYNOPKG_PKGDEST}/${cmd}" >> "${INST_LOG}" | ||
| ln -s "${SYNOPKG_PKGDEST}/${cmd}" "/usr/local/${cmd}" >> "${INST_LOG}" 2>&1 | ||
| mkdir -p "$(dirname /usr/local/${cmd})" 2>&1 | install_log |
There was a problem hiding this comment.
Can we remove the duplicate redirections everywhere? (your function would need to read both stdin and stderr but makes it clearer everywhere else)
| mkdir -p "$(dirname /usr/local/${cmd})" 2>&1 | install_log | |
| mkdir -p "$(dirname /usr/local/${cmd})" | install_log |
EDIT: Unfortunately It might not be possible, unless we forgo the date prefix string and just let the command print to stderr directly. The reason I think I almost prefer this as it simplifies a lot of code. I can imagine a scenario where most of the log commands are not needed.
Maybe we can use install_log to encapsulate all package scripts but allow a switch or function to disable it in case a package wants to notify the user. IMHO logs should always be written and not something a package author should worry about. At the moment packages have various syles/standards of logging. If a package needs to ignore logs for some reason they can still redirect to /dev/null
There was a problem hiding this comment.
Maybe we can use install_log to encapsulate all package scripts but allow a switch or function to disable it in case a package wants to notify the user. IMHO logs should always be written and not something a package author should worry about.
I like the idea to always write to the installer log file and it is easy to implement.
But I did not find any solution on how to disable this and write to stdout while all output is captured for the installer log (I tried to collect notification output in a global variable or to use file descriptor 3 - nothing worked).
The only solution I can imagine for user notification (i.e. writing to stdout) is to implement additional service functions that are not redirected to the installer log like validate_preinst or validate_preupgrade and that must be used for such validations.
This would work at least for validation of wizard variables, but not for complex installers that have multiple installation steps with extra validation (if we have any).
The real benefit of user notification is for preinst, preuninst and preupgrade anyway, as the user can navigate back in the wizard and fix the configuration before starting the installation/uninstallation.
Further a validate_postinst could notify when additional manual configuration might be required.
And for me it is on the high priority list, as a couple of packages fail to install (ambigous redirect) when I tried to build and install all packages for arch-x64-7.0 (see #4524) |
|
Oh, I don't remember single package to fail with that error. Thanks for fixing it. Before my NAS died, I had my own compiled python3 and mono installed. |
|
This is good work indeed. And, out of curiosity, I wonder if it could be possible to alter default Just a thought. |
Yes, this is my intention to update all packages, in spite of those that are WIP on branches (or - maybe it is better not to touch such packages and advise the maintainers after this one is merged). @th0ma7 regarding sc specific syslog configuration I propose to work on a different branch. After my redesign here there will be no need to modify each package for such a feature.... |
|
Unfortunately using the service_preinst ()
{
echo "****************"
exit 1
}I tried to grab it and pass it on with |
This works with the provided validate_ functions: validate_preinst ()
{
echo "****************"
exit 1
}So far I only found one package with user notifications that cannot be easyly migrated to validate-functions (nzbget/nzbget-testing). |
|
Sorry my point is that if a command fails then the script will continue to run instead of exiting. |
IMHO this was always the case. You must use |
|
@hgy59 It seems you are right. I just found it surprising that |
Page 115 of the dev guide, part of the resource list there are hooks available to add |
|
$ rg -g '**/*.sh' INST_VAR -l
spk/minio/src/service-setup.sh
spk/minio/src/wizard/upgrade_uifile.sh
spk/sonarr/src/service-setup.sh
spk/lidarr/src/service-setup.sh
spk/lidarr/src/service-setup-mono.sh
spk/radarr/src/service-setup.shI noticed another oversight on my part, the firewall rules on the DSM7 are also defined in the resource file. I'll happily send a PR spk.service.mk + ifeq ($(strip $(FWPORTS)),)
+ @jq '."port-config"."protocol-file" = "$(SPK_NAME)"' $@ 1<>$@
+ endifspk.service.installer.dsm7 - # Add firewall config
- if [ -r "${FWPORTS_FILE}" ]; then
- install_log "Installing service configuration ${FWPORTS_FILE}"
- servicetool --install-configure-file --package "${FWPORTS_FILE}" 2>&1 | install_log
- fi
- # Remove firewall config
- if [ -r "${FWPORTS_FILE}" ]; then
- install_log "Removing service configuration ${SYNOPKG_PKGNAME}.sc"
- servicetool --remove-configure-file --package "${SYNOPKG_PKGNAME}.sc" 2>&1 | install_log
- fi |
Yes, I noticed install errors like So shall I commit all the package fixes and then we merge it - or can I cherry pick the DSM7 firwall rules fix somewhere? For now I am holding back, as I have to test the installation of 61 packages (12 noarch/49 arch specific) on DSM 7 and DSM 6.1 and evtl. DSM 5.2. |
A search for |
|
The firewall rule is a bit more complicated than I thought. Using the resource worker it expects the firewall file to reside in
|
|
Sorry I had these changes locally and missed pushing them earlier. |
mk/spksrc.service.mk
Outdated
| @@ -196,10 +196,16 @@ ifeq ($(call version_ge, ${TCVERSION}, 7.0),1) | |||
| $(DSM_SCRIPTS_DIR)/installer: $(SPKSRC_MK)spksrc.service.installer.dsm7 | |||
| @$(dsm_script_copy) | |||
| else | |||
| ifeq ($(call version_lt, ${TCVERSION}, 6.0),1) | |||
There was a problem hiding this comment.
I think this could be written more consistently e.g. as greater than or equal to 7.0; dsm7 stuff, else greater than or equal to 6.0; dsm6 stuff else dsm5 stuff. Or the reverse. Nice work otherwise 👍 on separating the dns5 stuff.
There was a problem hiding this comment.
@publicarray i have a question regarding your comment in the servive.mk for DSM7
# STARTABLE needs to be yes, Resource linking and unlinking works on start and stop
I wanted to fix this, as only SPK_COMMANDS was tested here and SPK_USR_LOCAL_LINKS was ignored.
But my findings are, that it is not needed to omit ctl_stop=no (i.e. use default ctl_stop=yes) for resource linker to work.
The packages with ctl_stop=no are started by the system at installation time (and on every restart) and are stopped at uninstall.
This would simplify the code as it will depend on DSM >= 6.1 only to use ctl_stop=no or startable=no.
There was a problem hiding this comment.
@publicarray the implementation of ctl_stop=no is in spksrc.spk.mk and not in spksrc.service.mk but your comment is in both.
There was a problem hiding this comment.
Thanks for the optimisation. The downside is that most CLI apps have STARTABLE=no set. I just tested it again and CLI packages (I use zsh for testing) that have ctl_stop=no will still work but the start/stop button is obviously missing. IMHO we should override and remove the ctl_stop=no if SPK_COMMANDS is set. SPK_USR_LOCAL_LINKS doesn't guarantee an executable but yes it probably should also apply then.
Since this is a minor issue we can just ignore it for now.
spksrc on redesign_install_log [📦📝🤷]
❯ rg -g '**/*Makefile' --pcre2 -U --multiline-dotall '(?=.*STARTABLE = no)(?=.*SPK_COMMANDS)' -l | wc
61 61 1329Thanks SO: https://stackoverflow.com/questions/469913/regular-expressions-is-there-an-and-operator
|
@publicarray now I still only problems with demoservice on DSM7. Failed to install package, pkg=[demoservice]
result = [{
"action": "install",
"beta": false,
"betaIncoming": false,
"error": {
"code": 276,
"description": "failed to acquire postinst worker",
"worker_msg": []
},
"finished": true,
"installReboot": false,
"installing": true,
"language": "ger",
"last_stage": "postreplace",
"package": "demoservice",
"packageName": "DemoService",
"pid": 18788,
"scripts": [{
"code": 0,
"message": "",
"type": "preinst"
}, {
"code": 0,
"message": "",
"type": "postinst"
}
],
"spk": "/volume1/@tmp/upload_tmp.180930",
"stage": "install_failed",
"status": "stop",
"success": false,
"username": "xxxxxxxx"
}
]Did you ever get the error EDIT: |
|
I have added a note for packages using |
- use validate_preinst
- avoid use of /usr/local/{package-name}
Co-authored-by: Sebastian Schmidt <[email protected]>
- make wget less verbose to avoid download progress in log file
- dsm5: rename INST_LOG to INST_LOG_TEMP for unified INST_LOG
hgy59
force-pushed
the
redesign_install_log
branch
from
170460c
to
fc4c4ad
Compare
update for redesign of installer log (#4539)
|
@publicarray there is now some rework on the dsm7-packages branch required. for radarr in service-setup.sh in the function service_preupgrade:
TODO for transmission in service-setup.sh
I think by #4545 that tvheadend is outdated on the dsm7-packages branch (I posted a note in #4545). |
- revert overwriting files in ${SYNOPKG_PKGVAR} folder from target/var (introduced with SynoCommunity#4539)
* DSM7: revert var overwrite
- revert overwriting files in ${SYNOPKG_PKGVAR} folder from target/var (introduced with #4539)
- use different privilege files for DSM 7 and DSM < 7 - adjust installer logs (SynoCommunity#4539) - link ejabberdctl to target/bin
* update ejabberd - update ejabberd to 21.01 - prepare for generic service setup - activate install wizard * fix ejabberd service configuration and startup * fix SPK_COMMANDS * adjust ejabberd for DSM7 - use different privilege files for DSM 7 and DSM < 7 - adjust installer logs (#4539) - link ejabberdctl to target/bin
Motivation: Installation scripts using
>> ${INST_LOG}do not work with DSM7Linked issues: #4524, #4494, #4527, #4537, #4545, #4153
Checklist
Documentation that needs update:
Remarks
It is time to redesign the package installation logs, as appending output with
>> ${INST_LOG}is not supported with DSM7.In DSM7 write access to the new DSM installation log file
/var/log/packages/{package-name.log}is not allowed.With DSM7 all stderr output during installation is written to the logfile by the system and output to stdout is presented in a modal dialog to notify the user.
The purpose of this PR is to realize installation logging independently on the DSM version.
Finally the new function
install_logis used internally by the generic installer only.For all packages with custom installer functions (service_*) the only change is to remove the output redirections
>> ${INST_LOG}and>> ${INST_LOG} 2>&1.All output in the package specific installer functions will be written to the DSM dependent installer log file.
Packages that use
exit 1and/or user notifications in the installation process by writing to stdout, must use the new validate-functions to do so. User notification is not supported for DSM < 6.The new validate functions are:
The benefit of negative validation (exit1) is, that the user can navigate in the wizard to fix the installation parameters and try again. This might be
mysql administration password,shared folder that must exist, etc.For the rare case a package needs to download or install code before the validation, a validate function must be used and the install_log function can be used there for regular installation logs.
Installer log file
The install log with
INST_LOGwas introduced for pre DSM 6 packages, where DSM system did not create log file for package installation scripts.Finally the
/var/log/packages/{package-name}.logis not new for DSM7 but is provided since DSM6. So this PR modifies all DSM6 packages to use the/var/log/packages/{package-name}.logand do not create/var/package/{package-name}/target/var/{package-name}_install.loganymore.The standard for installer log is now
/var/log/packages/{package-name}.log.Only for DSM < 6 the
/var/package/{package-name}/target/var/{package-name}_install.logis still in use.Affected Packages
most of the Comments below are for building arch-x64-7.0
✔️: validation of installer log fixed for DSM7
✔️✔️: validation of installer log fixed for DSM7 and DSM6
fixed:
>> ${INST_LOG}/usr/local/{package-name}by/var/packages/{package-name}/targetnot fixed in this PR:
has not been updated to DSM7 yetas function ... is no longer supported.SERVICE_WIZARD_SHAREimplementation needs update vor DSM7localized stringfor several languagesfishnet_keysuanymoresuanymore, package is obsolete, asstable / testing (beta)is provided by the wizard of nzbget packageinstalling.../var/services/web/{package-name}