Forgot to mention, I was referring to the flatpak built from the gnome-apps-nightly repository [1], against the 3.22 runtime. Basically what you get in you currently install the "stable" GNOME Runtime [2]

That's a known issue and we know we need to address this, but I don't think bugzilla is the right place. It's not a simple question of bundling mission-control and idle, as connection managers are DBus activated.

So there are two options:
 - fix telepathy/idle to make it possible to bundle them
 - drop telepathy and use something else/do our own

We want to go with the latter, but that's a major effort, and I don't think it helps tracking it in bugzilla before there's any actual code (it will likely end up spanning multiple bugs anyway).

So unless you managed to bundle telepathy/idle and make it work (in which case I'll be happy to merge them as a short-term fix), it doesn't appear very useful to keep the report open.

As long as it's a known issue I think it's fine to close this bug as invalid, a my purpose when filing it was to make sure that this was acknowledged as, sadly, I don't think I will have time to work myself on this at any time soon.

Thanks for the prompt response, and for the clarification.

Created attachment 346096 [details] [review]
flatpak: Bundle mission-control/idle

Now that we ask for full session bus access for the room list, we can
just as well use that access to run our own copies of mission-control
and telepathy-idle in the case where those are not available on the
host.

The plan is still to get rid of telepathy altogether, but the crude hack attached above should allow us to not depend on host services in the meantime.

Oh! I agree this is a nasty hack but it's nice at the same time to have this alternative solution as a patch on top of the upstream code, and bundled all together in the same flatpak, so thanks a lot for doing that.

FWIW, I reviewed the patch and it looks good to me, all things considered, but I'm not sure who should be the person reviewing this change, so take this as a comment, not a formal review.

I guess my only question at this point would be... will you push the json file + the patch to the gnome-apps-nightly repository [1]? I know duplication is never a good thing, but pushing it there would mean having Polari installable without extra hassle from the "nightlies" repository, which should become the stable one as soon as we have a 3.24 runtime.

[1] https://git.gnome.org//browse/gnome-apps-nightly

Review of attachment 346096 [details] [review]:

I am getting some trailing whitespace errors in this one, perhaps it's the included .patch file, though.

::: org.gnome.Polari.json
@@ +37,3 @@
         "cflags": "-O2 -g"
     },
+    "cleanup": ["/include","/libexec/telepathy-logger","/lib/*.la","/lib/*.a",

So instead of cleanup "/libexec" the clean is performed on "/libexec/telepathy-logger" now? I dont understand this change in relation to the patch purpose, can you elaborate?

(In reply to Bastian Ilsø from comment #7)
> I am getting some trailing whitespace errors in this one, perhaps it's the
> included .patch file, though.

Yes. Diff adds a 1-character column at the beginning to mark added(+), removed(-) and unchanged( ) lines. So an unchanged empty line is represented as a line with a trailing space, nothing we can do there :-)


> ::: org.gnome.Polari.json
> @@ +37,3 @@
> +    "cleanup":
> ["/include","/libexec/telepathy-logger","/lib/*.la","/lib/*.a",
> 
> So instead of cleanup "/libexec" the clean is performed on
> "/libexec/telepathy-logger" now? I dont understand this change in relation
> to the patch purpose, can you elaborate?

The telepathy-logger modules contains both the actual logger and a library to interact with it. We're only interested in the library, so we remove the logger during cleanup. As it used to be the only file installed in /libexec, we could just clean the whole directory, but now mission-control and telepathy-idle install their executables there as well.

But now that I think about it, we should probably keep the logger executable as well and launch it manually as well if it's not available on the host ...

Thanks for the explanation.

I just tried completely removing polari, telepathy and telepathy-idle etc from my system to see if this works, but im running into this issue:

(org.gnome.Polari:3): Gjs-WARNING **: JS ERROR: TelepathyGLib.Error: Protocol 'irc' not found on CM 'idle'
ConnectionsList<._onRowActivated/<@resource:///org/gnome/Polari/js/connections.js:170:21
main@resource:///org/gnome/Polari/js/main.js:46:5
run@resource:///org/gnome/gjs/modules/package.js:192:25
start@resource:///org/gnome/gjs/modules/package.js:176:5
@<main>:1:146

Note that I do see telepathy installed inside the flatpak here:
/libexec/telepathy-idle
/libexec/mission-control-5
/libexec/telepathy-logger

any ideas?

(In reply to Bastian Ilsø from comment #9)
> Note that I do see telepathy installed inside the flatpak here:
> /libexec/telepathy-idle
> /libexec/mission-control-5
> /libexec/telepathy-logger

Hmm, are you sure about that? Those should be in /app/libexec/...
Other than that, my only idea would be some of those uninstalled services still running in the session and getting somehow confused ...

I installed the Builder nightly flatpak in a VM, and I think I figured it out - builder uses the flatpak manifest to build all modules *up to* the project itself, then builds the project from the local source checkout. In other words: The patch to start mission-control/idle is never applied, and so of course it's not working.

Running "git am build-aux/flatpak/app-*" from the built-in terminal fixes it, but it means that this patch only works for the distributed flatpaks, not for doing flatpak-based polari hacking :-(

I'll come up with an updated patch.

Created attachment 346168 [details] [review]
flatpak: Bundle mission-control/idle

We currently rely on various telepathy services to be available on the
host - namely mission-control, idle and telepathy-logger. GNOME's builtin
chat integration means that mission-control and logger will likely be
available on GNOME systems, but idle is rarely if ever included in
default installs - and of course for non-GNOME systems, all bets are
off anyway.
To address that situation, bundle the required services in the sandbox.
They won't be visible to DBus' StartServiceByName method, but we'll
add code to launch the demons as necessary in a follow-up patch.

Created attachment 346169 [details] [review]
app: Manually launch telepathy demons if necessary

When running in a flatpak sandbox, we shouldn't rely on the required
telepathy services being available on the host; instead, detect the
case where they are missing and launch the bundled demons manually.

Review of attachment 346168 [details] [review]:

looks good to me

Review of attachment 346169 [details] [review]:

i read the code and i think it looks ok (i also tested the code and it works for me). i have some questions out of curiosity though

::: src/application.js
@@ +121,3 @@
+    },
+
+    vfunc_dbus_unregister: function(conn, path) {

just out of curiosity, why does vfunc_dbus_register require you to return true, while vfunc_dbus_unregister doesn't?

::: src/utils.js
@@ +83,3 @@
+function isFlatpakSandbox() {
+    if (_inFlatpakSandbox === undefined)
+        _inFlatpakSandbox = GLib.file_test('/.flatpak-info', GLib.FileTest.EXISTS);

we can guarantee that this file will also be present in a flatpak sandbox? is this the recommended way to check whether we are in a sandbox?

(In reply to Bastian Ilsø from comment #15)
> just out of curiosity, why does vfunc_dbus_register require you to return
> true, while vfunc_dbus_unregister doesn't?

See the documentation of those vfuncs:
https://developer.gnome.org/gio/stable/GApplication.html#GApplicationClass

In short, if the dbus_register() function returns false, then the registration will be aborted - that is, it is a convenient way to interrupt application startup in case of error.
When dbus_unregister() is called on the other hand, the application is already shutting down - interrupting that and letting the application hang half-way through shutdown doesn't make sense, in particular in the error case.


> ::: src/utils.js
> @@ +83,3 @@
> +function isFlatpakSandbox() {
> +    if (_inFlatpakSandbox === undefined)
> +        _inFlatpakSandbox = GLib.file_test('/.flatpak-info',
> GLib.FileTest.EXISTS);
> 
> we can guarantee that this file will also be present in a flatpak sandbox?

Well, if the file is present and we are *not* in a flatpak sandbox, we will be very confused. But yes, when running under flatpak, the file is expected to exist  - see http://git.gnome.org/browse/mutter/commit?id=bccff5bdd85c55 for example.


> is this the recommended way to check whether we are in a sandbox?

I was about to say yes, but apparently GTK+ checks for $XDG_RUNTIME_DIR/flatpak-info instead (which is actually a symlink to the hidden file in /). If there is any value in tricking us into thinking we are inside a sandbox when we are not, then keeping the existent checks looks slightly safer as it requires root instead of user permissions. But then I can't think of anything to exploit there, so meh ...
I'll check with Alex :-)

ah ok, thank you for the explanations.

Running with these patches for a while now and in a flatpak on a host system without telepathy, I notice that Polari doesn't seem to do a very good job remembering what rooms I am joined to across sessions. Yesterday I joined 30 rooms on Freenode and GNOME. Starting Polari again today, I arrive at the empty state with no rooms joined or networks configured. Any ideas?

Mmh, not really. Saved rooms for deleted networks are cleared out(*), so when we lose the configured accounts, we lose the rooms as well. That leaves the question of why the networks are gone in the first place. That's something I cannot answer without investigation, but some thoughts anyway:

Mission-control stores accounts in $XDG_DATA_HOME/telepathy/mission-control/accounts.cfg; we don't map that into the sandbox, so the values for XDG_DATA_HOME (and thus the configuration used) will be different between system and flatpak version.
In practice, this shouldn't be much of a problem though, considering that we will use the telepathy services from the system when available. It is an issue though when setting up networks with "system tp", uninstalling it and then start fresh with "sandbox tp". Maybe this is what happened here?

However if you did set up networks and rooms with the bundled services, then we have a graver problem, but I'm not sure what that would be. At least from the flatpak side, the sandbox xdg-dirs are preserved between runs. Maybe Builder whipes them somewhere in its build pipeline to ensure a clean state?

(That would be quite bad actually, as the directories are tied to the app ID, that is they are shared between polari flatpaks from all repos (say, gnome-nightly-apps and gnome-builder)).



(*) Account names don't carry any information about the network, they are basically "idle-specific-prefix + number". A new account will always take the lowest available number, including those that were used by previously removed accounts. So keeping any associated information would be quite confusing (#gnome-design on Freenode etc.).

If I start polari and join some rooms, then quit polari via the app menu and start it again, the rooms and networks stays around.  This seems to be because that even if I quit polari, mission-control-5, telepathy-idle etc. are still running. If I kill those processes and start polari again, the networks and rooms that I have joined are gone. When I look in /.var/app/org.gnome.Polari I see the following files only:

cache/fontconfig/*
cache/polari/close-confirmation-shown
cache/polari/is-first-run
telepathy/logger/sqlite-data
tmp/ (empty)
config/autostart/org.gnome.Polari.Autostart.desktop
config/user-dirs.dirs
data/ (empty)


Perhaps there is an issue with file permissions, access or so here?

Note that i am running the flatpak outside of Builder using flatpak run org.gnome.Polari (the flatpak was initially built using Builder though).

(In reply to Florian Müllner from comment #16)
> (In reply to Bastian Ilsø from comment #15)
> > is this the recommended way to check whether we are in a sandbox?
> 
> I was about to say yes, but apparently GTK+ checks for
> $XDG_RUNTIME_DIR/flatpak-info instead [...]
> I'll check with Alex :-)

I did that yesterday, and /.flatpak-info is indeed the recommendation.


(In reply to Bastian Ilsø from comment #19)
> If I start polari and join some rooms, then quit polari via the app menu and
> start it again, the rooms and networks stays around.  This seems to be
> because that even if I quit polari, mission-control-5, telepathy-idle etc.
> are still running.

That is unexpected - the code in dbus_unregister() is supposed to get rid of them.


 If I kill those processes and start polari again, the
> networks and rooms that I have joined are gone. When I look in
> /.var/app/org.gnome.Polari I see the following files only:
> 
> [...]
> data/ (empty)

That's also unexpected - that's where mission-control is expected to store the accounts.


> Perhaps there is an issue with file permissions, access or so here?

Maybe. I've done some further testing, and it works as expected here:

 (1) uninstall mission-control/idle and make sure they aren't running
 (2) gsettings reset org.gnome.Polari saved-channel-list (*)
 (3) rm -rf ~/.var/app/org.gnome.Polari/data/telepathy
 (4) set up a network, join some channels, quit
 (5) confirm mission-control/idle are not running, account data
     exists in ~/.var/app/org.gnome.Polari/data/telepathy
 (6) start polari again, connects to the previously configured
     network and joins saved channels

So really no idea what's going on with your setup :-(


(*) Pro tip:
    $ gsettings get org.gnome.Polari saved-channel-list > saved-channels
    (before testing)

    $ gsettings set org.gnome.Polari saved-channel-list "$(<saved-channels)"
    (after testing)

I still dont know exactly what is going on with my setup and it doesn't help either that I just upgraded gnome-builder with the new build system. However, i think pushing these patches and having something is better than being completely broken like right now, as you also said in IRC.

Attachment 346168 [details] pushed as 6988948 - flatpak: Bundle mission-control/idle
Attachment 346169 [details] pushed as a0f21c8 - app: Manually launch telepathy demons if necessary