Add --replace-all option for development
#751
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This action was registered, but did not work because the handler accepted too few arguments and also called itself recursively. This fixes both, calling the 'quit' action (as exposed through dbus by gtk automatically) now actually quits the GUI.
This simplifies the flow a bit by putting the handling of "add" with arguments at the same level of all other application actions instead of below it, needing one fewer level of indentation. Behavior should be fully unchanged. Diff best viewed while ignoring whitespace changes.
This logs the message after the storage class was actually instantiated and registered on the bus, rather than before.
This converts the single print in this script to use a logger instance (like hamster-service already uses).
This refactors the way that dbus names are claimed for hamster-service and hamster-window-service. Instead of checking if the name is already claimed beforehand, this tries to claim the and checks the result. This fixes a minor race condition when a service is started twice at the same time. Before, the name would appear to be free when it was checked, but when the script proceeded to actually claim the name, it could have been claimed by another instance already. Because do_not_queue was not yet used, this means that the claim would probably not even fail, but leave a pointless process running. This commit refactors the code structure to introduce a claim_bus_name helper, which claims the name with do_not_queue=True so it either fails when the name is already taken, or is sure the name is claimed. This also raises the bus claiming code out of the Storage and WindowServer classes into the main script, since that is a better place to decide to abort startup and log messages about this. This refactoring also prepares for implementing a `--replace` option in a subsequent commit.
This makes them replace any currently running GUI, storage service or windows service. This is primarily useful during development, to prevent having to kill these services manually. For the two services, this is implemented properly by trying to claim the bus name and if it is taken, calling the quit method on the current instance while the existing claim is kept in the dbus request queue. This ensures that as soon as the name is released, it is claimed again by the new process (preventing a race condition where a service autostart could occur in between). For the GUI, this race condition is not prevented due to the way Gtk/Gio.Application claims its name, but this should be minor enough an issue to not be problematic (especially since the user can always easily quit the GUI manually beforehand). This commit implements most of projecthamster#746.
This makes the filenames equal to the name they get when installed, which is more consistent and makes it easier to call them programmatically (this prepares for implementing `--replace-all` in subsequent commit). The .py extension was added in commit 774c315 (give .py extension to hamster-*), on account of letting xgettext understand these are python files. Currently, there is no working (or at least not documented) xgettext flow anyway, and xgettext seems to support passing an explicit language parameter, so this can probably be used, removing the need for these explicit extensions.
This reverts commit 767f80a22593757aec10b3f6ab3a4ef3ab8a0809.
This lets the service commands (hamster-service and hamster-windows-service) daemonize after initialization. This ensures they are detached from their parent process, and allows a caller to wait for them to return to know whether startup succeeded or not.
This replaces the GUI and two background services (by using the recently added `--replace` options). This is helpful during development to easily replace the existing deamons (even when installed system-wide) with a development version, without having to resort to fragile pkill commands. This also updates the README to recommend using this option instead of kill commands. This also removes a note about not calling windows via dbus, since that does not seem to be true with the current codebase. This fixes projecthamster#746
|
Automatically generated build artifacts for commit 4109d79 (note: these links will expire after some time):
To test this PR, download and unzip the flatpak application above and then install with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a
--replace-allcommandline option to make hamster send quit commands to all running services and replace them with new version. This is convenient during development, to test a new version without having to manually kill the old version (and hope it is not autorestarted by dbus in the meantime).I'm marking this as a draft, since the daemonization added seems to interfere with the glib mainloop somehow.
What I did was have the hamster-service and hamster-windows-service commands daemonize after startup (e.g. after they successfully claimed their dbus name, or gave up trying to). This allows
hamster --replace-all, which callshamster-service --replaceandhamster-windows-service --replace, to wait until both background services are started before continuing, and also report failure if either one fails to be replaced.However, it seems that doing the
fork()call needed for this after thefrom gi.repository import Gtk as gtkimport (which is done inhamster/__init__.pyearly for "performance"), breaks some Glib internals (I noticed this because the file monitor for changes to the python source stopped working, and later also got timeouts during startup (Error creating proxy: Error calling StartServiceByName for org.gtk.vfs.Daemon: Timeout was reached (g-io-error-quark, 24)).I'm not quite sure how to resolve this yet - Messing up glib is obviously unacceptable. Reordering things so the fork happens before the Gtk import, but still after startup (or at least after dbus name claim) could work, but is not going to pretty and is probably fragile for future refactoring. Not doing the fork at all seems the most likely option, but that could mean
--replace-allwould just have to do a fixed timeout, or wait for some message on stdout or another file descriptor, or a signal or some other IPC, which are not so nice and/or more complex. Maybe waiting for a dbus NameOwnerCange could work (and maybe we could match the pid, or just be ok with any change) and be somewhat elegant.This needs a bit more thought, but I'm out of time for today (and tomorrow). I did want to share the progress so far, hence this PR.