Skip to content
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

Logging improvements #1456

Merged
merged 1 commit into from
Mar 13, 2019
Merged

Conversation

bitdisaster
Copy link
Contributor

This PR brings various improvements around logging with the main purpose of making them easier to understand for developer or CE engineers that do not have knowledge of the internals of Squirrel.

1.) Bugfix because of multiple process logging to the same file
With exemption of the --uninstall case logs get written to the location of the executing binary. During install and update scenarios the Update.exe in %localappdate%\MyApp can run in multiple process at the same time due to squirrel hooks. Previous code has tried to avoid collision but since the file was opened with ReadWrite access a collision would never happen. Instead partial or all content from one or the other process was missing. While the collision code is fixed and remains, a new naming strategy will also avoid collision (see 2.)
2.) Better Squirrel log naming
Squirrel logs leaves logs in multiple locations depending on the executing binary's location. Furthermore, log file can contain information from multiples runs of different Squirrel commands such --update, --createShortcut, etc. To make it easier to understand the content log files are now named by the invoked Squirrel command, e,g Squirrel-Update.log, Squirrel-Uninstall.log. The naming also avoids collision described in 1.)
3.) No errors/ exceptions in happy path
Even if an installation or update went perfectly, the logs contained errors/exceptions. Without deeper knowledge of Squirrel its not easy to understand whether this is of any concern. From now on the logs will be nice and shiny if Squirrel walked the happy path.
4.) Clear end of logging session
When reading the log files its not clear whether a command such as --install was executed successful. It seems like a rather sudden stop. A final log entry at the end adds some peace of mind.
5.) Add log level
While Squirrel uses log level internally they were not written to the log files. Now we will know if an entry is just and info or a warning or an error.

@@ -536,25 +536,25 @@ internal void unshimOurselves()
// directory are "dead" (i.e. already uninstalled, but not deleted), and
// we blow them away. This is to make sure that we don't attempt to run
// an uninstaller on an already-uninstalled version.
async Task cleanDeadVersions(SemanticVersion originalVersion, SemanticVersion currentVersion, bool forceUninstall = false)
async Task cleanDeadVersions(SemanticVersion currentVersion, SemanticVersion newVersion, bool forceUninstall = false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a much better name, 👍

@@ -0,0 +1,8 @@
namespace Squirrel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squirrel doesn't use one-class-per-file, can you put this in IUpdateManager.cs (or maybe a different file, whatever makes Sense)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thing

@@ -0,0 +1,85 @@
using Mono.Options;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Into this refactor 👍

@anaisbetts
Copy link
Contributor

anaisbetts commented Mar 13, 2019

Can you rebase against upstream master to get rid of the weird merge commits? (lemme know if this gives you Grief and I can help out)

@bitdisaster bitdisaster force-pushed the logging_improvements branch 2 times, most recently from 81d7365 to 08f71bf Compare March 13, 2019 22:02
@bitdisaster
Copy link
Contributor Author

Yeah, those commits refuse to go away. You are not on Electron HQ anymore

@bitdisaster bitdisaster force-pushed the logging_improvements branch from 08f71bf to 4953432 Compare March 13, 2019 22:13
@bitdisaster
Copy link
Contributor Author

Got it done. All feedback addressed so far.

anaisbetts added a commit that referenced this pull request Mar 13, 2019
@anaisbetts
Copy link
Contributor

anaisbetts commented Mar 13, 2019

@bitdisaster Wait one more thing, this PR breaks a public interface, can we find a way to do this without the breaking change to CheckForUpdate? Generally if the user doesn't say otherwise they're in the update scenario, the initial install scenario is generally only used internally

@bitdisaster
Copy link
Contributor Author

🤔

@bitdisaster
Copy link
Contributor Author

I can't think of way to get totally rid of it because at the place where I need it, I have no other context on whether its an update or install scenario. However, we could make it virtually a non breaking change by moving the parameter to the end and apply a default value. Task<UpdateInfo> CheckForUpdate( bool ignoreDeltaUpdates = false, Action<int> progress = null, UpdaterIntention intention = UpdaterIntention .Update);

@anaisbetts
Copy link
Contributor

However, we could make it virtually a non breaking change by moving the parameter to the end and apply a default value

Works for me, we don't promise ABI compatibility

@bitdisaster bitdisaster force-pushed the logging_improvements branch from 9c452ad to 073fac7 Compare March 13, 2019 23:04
@bitdisaster bitdisaster force-pushed the logging_improvements branch from 073fac7 to 1516a3a Compare March 13, 2019 23:04
@anaisbetts anaisbetts merged commit b7b4a84 into Squirrel:master Mar 13, 2019
@anaisbetts
Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants