-
Notifications
You must be signed in to change notification settings - Fork 176
Improve logging for EXE installers #1108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@jaimergp, this can get a preliminary review, but I will keep it in draft until conda/conda-standalone#218 is merged. |
constructor/nsis/main.nsi.tmpl
Outdated
| ${If} ${Silent} | ||
| push '"$INSTDIR\pkgs\post_install.bat" > "${STEP_LOG}" 2>&1"' | ||
| ${Else} | ||
| push '"$INSTDIR\pkgs\post_install.bat"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we do something like <command> > output.txt & type output.txt? As in "output to file, then print the file contents".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what we're doing for silent installations. This would mean that we don't see the output in real time though. If you think the file logging is more important, I can change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we gain too much by having real time output in the post install scripts (they should usually be quite quick to run). The other idea is to use powershell's Tee-Object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping here @marcoesters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't as trivial as it seemed at first because type changes the exit code such that commands always succeed. I found a workaround - not pretty, but it works.
Description
The logging experience for EXE installers has several problems that make debugging difficult to impossible:
install.logfiles to appear inside%USERPROFILE%or even cause the installation to fail because the check for empty directories fail due to the log file.icacls.exeorpython.exe) fails.This PR addresses these issues by:
$INSTDIRhas been created.cmd.exesubshell to output error messages coming from executing the binary (e.g., if it's not found or if there are permission issues). This makespythonw.exeobsolete and outputs errors from the custom Python script.AbortRetryNSExecWaitso that logs are still available if the uninstallation fails (continuing even though steps fail shouldn't be happening in the first place to avoid undefined behavior, in my opinion).I also decided to remove unused functions, which resulted in
Utils.nshonly containing theIsWritablefunction. I could move that function into the main template, or I can move the functions inside theFunctionTemplatesmacro and thePrintmacro intoUtils.nsh.Caveats:
condacommands cannot directly write intoinstall.logeven if they support--log-filebecause NSIS maintains the file handle on the log file and doesn't allow writing to it outsideLogSet.cmd.exedoes not support theteecommand, there is a trade-off between live printing for GUI installers and logging for system calls. I prioritized getting real time output into the GUI window over writing to the log file.This also closes #998
Xref: conda/conda-standalone#218
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?