snapcraft.yaml: add support for building a snap package

[3v1n0]

The package is generated to be completely confined right now, and this might cause some minor annoyances, for example: it's not possible to access (sync) .dotted folders, the home is in the ~/snap/nextcloud-client, password can't be saved to the system keyring.

If you prefer not have these limitations, and make this client to run without any sandbox, we can move to the classic confinement, and this will work as it used to be.

The build can also be easily integrated with travis-CI and you can upload the new revisions automatically to the 'edge' channel of the snap store.

If you need any help for this, I'm available.

[enoch85]

@kyrofa Is this correct? Can you review please?

[kyrofa]

Thanks @3v1n0! Good work here, although I have a number of suggestions/comments/questions. And perhaps the kicker: this doesn't run at all for me, it just seems to hang. Not sure if that's because of the snap or because I'm trying to run master, though.

[kyrofa]

At least for the time being, Ubuntu Software shows this field as the title. I suggest capitalizing it: "Nextcloud Desktop Client."

[kyrofa]

I'm gonna go out on a limb and say you probably want something in ../nextcloudtheme/theme/colored/ 😉 .

[kyrofa]

s/ownCloud/Nextcloud/, no?

[kyrofa]

Personally, until LP: #1653769 is resolved, I'd prefer classic confinement. Password prompts at start-up make it too annoying for me to use.

[3v1n0]

Ok, agree... Still not sure if devmode is better though as an interim step to the final confined (if that is ever wanted).

[kyrofa]

g++ is missing here.

[kyrofa]

This doesn't actually pick up the theme (which is why, as you may have noticed, everything still says ownCloud). -DOEM_THEME_DIR=$SNAPCRAFT_PART_INSTALL/../src/nextcloudtheme will make you happier. Note that you'll need to update other sections in the YAML (such as the .desktop file and LD_LIBRARY_PATHs) that are set for something related to ownCloud.

[kyrofa]

Can you explain why indicator-qt5 is required? I was able to get the icon working fine simply by setting TMPDIR.

[kyrofa]

Take a look at this (and test with sudo snap install --edge nextcloud-client-kyrofa on amd64) to see what I did specifically. I'm curious what this part adds on top of that. As I mentioned, I couldn't get this to run, so I was unable to see for myself.

[3v1n0]

That is an "hack" that I actually was using (although now you should now use TMPDIR set as XDG_RUNTIME_DIR, as that is guaranteed to be cleaned up), however when snaps are confined that's not enough because this also would try to connect to dbus to get if running under unity or not.

There's an SRU for appmenu-qt5 in progress which will make this optional, but in the mean time that is the cleanest solution for this problem (see trunk appmenu-qt5 code).

[kyrofa]

Sounds good, thanks.

[kyrofa]

Can you explain this part, please? Obviously snapd-xdg-open needs to run as an external "trusted helper" if you will (i.e. outside of confinement). You're just copying a piece of it so as to not use the one xdg-utils, it seems, but I was under the impression that nothing "special" was needed within a snap to use snapd-xdg-open as long as it was installed on the system. Is that not the case?

[3v1n0]

This kind of xdg-open right now is needed in order to be able to call that trusted service.
At least, without it there's no way for me to open links (while file:// protocol is not supported unfortunately) when confined.

[kyrofa]

Fair enough, thank you 😃 .

[kyrofa]

I just saw this, by the way. Can you use the one in /usr/local/bin of the core snap?

[kyrofa]

No scratch that, it's not available until 2.23. Carry on.

[3v1n0]

Eh, in fact that's why I didn't do it :-)

[kyrofa]

This wasn't picked up for me (i.e. doesn't show in the dash). Did you test this?

[enoch85]

Can you please fix the rest @3v1n0 Thanks! :)

[3v1n0]

@kyrofa

And perhaps the kicker: this doesn't run at all for me

Ah, I've noticed that too when you only build it confined.
If you run it in --devmode and then confined it works, but indeed this is a blocker.

So, I'm moving to classic, while probably using just devmode would be fine. It all depends how we want this to interact with the system, and if the idea of having this confined is the final target.
In general using a NextCloud folder + symlinks would be enough to sync everything even when in devmode (and ultimately confined)...

So @enoch85 I guess we need your word and let us know how you prefer the snap to be.

[kyrofa]

Ah, I've noticed that too when you only build it confined. If you run it in --devmode and then confined it works, but indeed this is a blocker.

Wait... what? Why? The example client snap to which I linked runs in confinement. What's missing here?

[3v1n0]

@kyrofa oh, sorry that was because it was missing the network-bind interface... So, yeah it works confined.

So let's decide if for now we prefer it to be classic or devmode.

[enoch85]

@kyrofa Can you please have another look, this is more your area than mine.

Thanks!

[kyrofa]

Why is network-manager required?

[3v1n0]

See this bug, Qt network status checker needs it, and so this client if it wants to know the connection status.

This is not an auto-connected interface, though.

[kyrofa]

Ah, okay. It still works if the interface is not connected, though? What features do we lose by not having it automatically connected?

[3v1n0]

Things will work properly anyway... I'm not sure if this client is actually using this feature, but consider that as an "optimization". When Qt knows if there's a connection or not, it won't try to transfer data.

Users will be free to manually connect to this interface if they want an optimized client though.

[kyrofa]

Good deal.

[kyrofa]

What is the purpose behind this? The commit mentions remembering config, but I don't have that issue with nextcloud-client-kyrofa. Am I misunderstanding?

[3v1n0]

Without this, the client won't be able to access to $SNAP/etc/Nextcloud/sync-exclude.lst (which causes syncing not to work here; I deleted the log stating it, but if you want, I can paste it), the same with any other possibly hardcoded path. We need to be sure that the client will be able to access to such files.

[enoch85]

cc @kyrofa

[kyrofa]

Have you tried simply -DSYSCONF_INSTALL_DIR=config (or etc if you prefer, though that meaning breaks down in a snap)? That works for me.

[3v1n0]

I tried that, but then my snap was trying to access located in $PATH_WHERE_I_WAS_BUILDING_THIS_SNAP/part/client/build/config not sure why...

[3v1n0]

Also translations are there... For example:

for i in $(grep /snap/nextcloud-client prime/ -rl); do echo - $i:; strings $i | grep /snap/nextcloud-client; done gives me:

- prime/usr/bin/nextcloud:
/snap/nextcloud-client/current/usr//snap/nextcloud-client/current/usr/lib/nextcloud
/snap/nextcloud-client/current/usr/share/nextcloud/i18n/
- prime/usr/bin/nextcloudcmd:
/snap/nextcloud-client/current/usr//snap/nextcloud-client/current/usr/lib/nextcloud
- prime/usr/lib/libnextcloudsync.so.2.3.0:
/snap/nextcloud-client/current/usr//snap/nextcloud-client/current/usr/lib/nextcloud
/snap/nextcloud-client/current/etc/%1
/snap/nextcloud-client/current/etc/%1/%1.conf

Let me see what happens using your way...

[3v1n0]

I've tried that too, but it's not fine, the same command (with different pattern) gives me:

/media/marco/M.2-SSD/client_theming/linux/parts/client/build/client/config/%1
/media/marco/M.2-SSD/client_theming/linux/parts/client/build/client/config/%1/%1.conf

where /media/marco/M.2-SSD/client_theming is the local repo path.

[3v1n0]

I've tried that too, but it's not fine, the same command (with different pattern) gives me:

/media/marco/M.2-SSD/client_theming/linux/parts/client/build/client/config/%1
/media/marco/M.2-SSD/client_theming/linux/parts/client/build/client/config/%1/%1.conf

where /media/marco/M.2-SSD/client_theming is the local repo path.

[kyrofa]

You say you get a log denying access. Do you still get that message? Do you get that message with nextcloud-client-kyrofa?

[3v1n0]

Mh, no... I can't either test what's the log saying though since the run-clientis missing a $*.
I'll try to build it locally

[enoch85]

cc @kyrofa Can you please review?

[enoch85]

After discussing with @3v1n0 on IRC I think we should go for strict confinement because: Snaps in developer mode can not be released into the stable and candidate store channels. Though according to him there is a bug that makes the client ask for the user password every time the snap (client) starts which is not ideal. Maybe you could point out that bug so that more people could have a look?

Anyway, would be cool to get this merged asap. Waiting for what @kyrofa has to say.

[3v1n0]

This is the bug for the password saving.

[kyrofa]

I think we should go for strict confinement because: Snaps in developer mode can not be released into the stable and candidate store channels. Though according to him there is a bug that makes the client ask for the user password every time the snap (client) starts which is not ideal.

This is exactly why I was suggesting classic confinement, which allows for no password prompt and release to stable channels at the expense of confinement. My only question is: what happens to the currently-installed snaps (using classic confinement) if that keyring interface lands and we move to strict confinement? Bad things?

If we're okay with the password prompt until that interface lands, then I'm all for strict confinement. But like I said, the prompt at startup will cause me to prefer the deb version.

[3v1n0]

If we're okay with the password prompt until that interface lands, then I'm all for strict confinement. But like I said, the prompt at startup will cause me to prefer the deb version.

I agree with you @kyrofa, but at the same time releasing with classic interface would need more work for now (as the desktop-helper needs some love too, as some things such as LD_LIBRARY_PATH aren't set and there would no need to repack the system themes for example)...

Also, if we release with classic, we still need to set the $HOME to $SNAP_USER_DATA because otherwise moving to confined will cause the users the trouble to move their current synced dir to somewhere else without any notice.

Instead by doing this change now, we can just say this to all the users that will install the snap. So they will know it and nothing will change.

[kyrofa]

Instead by doing this change now, we can just say this to all the users that will install the snap. So they will know it and nothing will change.

Sounds good. I can't imagine that keyring interface will be that difficult, anyway.

[3v1n0]

Sounds good. I can't imagine that interface will be that difficult, anyway.

Not really (and I'm looking into that), but... There still could be concerns as once the user has unlocked the keyring and if he saves the informations for the session, then I think that any app could access to such credentials (I've to check the APIs better though).

[kyrofa]

There still could be concerns as once the user has unlocked the keyring and if he saves the informations for the session, then I think that any app could access to such credentials [...]

Indeed, though if that's true it'll likely be an interface that is not automatically connected, which still works. That's up to the security team, anyway 😉 .

[enoch85]

So strict it is then?

What is left do do before this can be merged?

[enoch85]

@oparoz Can you please also take a look at this? You need to register the name under the Nextcloud store account, and afaik you are the only one with access.

I also suggest that we release this in the same phase as the ordinary ownCloud client, but without betas.

[kyrofa]

@3v1n0 how do you envision this being released? Since the snapcraft.yaml is hidden away, I don't think Launchpad will build this.

[3v1n0]

@kyrofa check this branch: https://github.com/3v1n0/nextcloud-client_theming/blob/travis-snap-build/linux/travis-build.sh (and the relative .travis.yaml)

Also I think we'd need a way to tell launchpad where the snapcraft.yaml can be found. As it's not always in snap or root folder.

[3v1n0]

@enoch85 strict is fine for me, if we really have troubles with it, we can go with something more permissive...

As for release, the branch linked above, will release any git revision to edge. As for manual releases, to stable, it's up to you guys to define the rules.

PS: @oparoz you can add multiple accounts to manage a snap, in the store collaboration section.

[3v1n0]

If you want to test a built snap in this way, here's a binary: https://transfer.sh/XW1As/nextcloud-client-2.2.4-git-amd64.snap

[oparoz]

@3v1n0 We can make it so that Nextcloud is the publisher of the snaps you create.

[3v1n0]

@oparoz of course... That's the plan!
See this page, you need to register the snap name (I chose nextcloud-client but you can change it if you want), and then we should make travis to upload the snap on each git release (I've already a branch for that) to the edge channel.

But all the snap concept is around the fact that you (upstream) will then manage the releases...

No others will be able to upload it then.

[oparoz]

I registered nextcloud-client. Travis can't build the snap, we need to trigger builds on Launchpad AFAIK.

[3v1n0]

@oparoz good..

Travis can't build the snap, we need to trigger builds on Launchpad AFAIK.

Of course it does: https://travis-ci.org/3v1n0/nextcloud-client_theming/builds/204436924

[oparoz]

Ah, yes, we don't run into the same timeout issues we have with the nextcloud server snap.

So what are the next steps then?

1. Merge this
2. Set up Travis script
3. Manually upload the first build
4. Give you access as a collaborator
5. Automate the uploading

?

[kyrofa]

Will you be able to build for both amd64 and i386 on Travis?

[3v1n0]

@kyrofa of course, it's already doing it, see the travis matrix.

[3v1n0]

@oparoz

Once we decided if you want to go with devmode or strict confinement (I guess you accept for the strict limitations for now) the steps are

1. Merge this (and generate the snaps you want to upload manually)
2. Wait for my new PR that adds the travis building
3. Generate the .snapcraft.cfg file and save the credentials at travis level to make travis to automatically upload the file (you can do it with snapcraft enable-ci travis)

I don't need to be a collaborator for the snap, as you can manage that alone, but I can help if you want...

PS: @kyrofa please re-consider your review as that issue seems to be addressed.

[kyrofa]

@kyrofa please re-consider your review as that issue seems to be addressed.

It has not. You still haven't answered my question.

[3v1n0]

@kyrofa which one you mean, am I missing something? The one related to xdg-open seems replied to me, or what do you miss?

[kyrofa]

@kyrofa which one you mean, am I missing something?

Sorry, I added the link in an edit. I don't like the absolute path hack, and the spike I built seemed to work without it. You imply that there are issues, but you haven't addressed why those issues don't seem to be present in my spike. Perhaps they are present and I'm not seeing them for some reason, but you haven't addressed that either.

[3v1n0]

@kyrofa ah, ok... Since the review was pointing to something else.
Let me try building your snap, however as I said during compilation there are some absolute paths that are hardcoded, and we have to ensure that these will point to the right place.

[kyrofa]

@kyrofa ah, ok... Since the review was pointing to something else.

Yeah I don't typically "request changes" multiple times. The red X is already there 😛 .

Let me try building your snap, however as I said during compilation there are some absolute paths that are hardcoded, and we have to ensure that these will point to the right place.

Or you can just install from edge as I've suggested. The snapcraft.yaml will pull a tag that doesn't build without modifications (i.e. it was pre-this-PR).

[3v1n0]

Yeah @kyrofa, I already installed it from edge, but as said, I wasn't able to see the full log since you can't pass arguments to that version... So I wanted to rebuild it, however even the snap in the store points to the wrong conf path:

/home/ubuntu/nextcloud-client-snap/parts/client/build/client/config/%1
/home/ubuntu/nextcloud-client-snap/parts/client/build/client/config/%1/%1.conf

Building is the same, of course [as for the GIT_SHA edit, I already figured that, but thanks :-)]...

PS: sorry I've just noticed I didn't submit some comments

[3v1n0]

@kyrofa so I've just rebuilt your snap... It works when using version (git tag) 2.2.4, not with master:

See this log: http://paste.ubuntu.com/24127025/ :
03-06 22:32:55:921 2679 OCC::Folder::setIgnoredFiles: ==== adding system ignore list to csync: "/media/marco/M.2-SSD/nextcloud-client-snap/parts/client/build/client/config/Nextcloud/sync-exclude.lst"

And the client error:

[kyrofa]

@kyrofa so I've just rebuilt your snap... It works when using version (git tag) 2.2.4, not with master:

Ah ha! That's what I wanted to know! That explains the discrepancy. I wonder what changed.

With that in mind, I'm okay with this, but I very very much dislike the hack. We should consider patches to make the client more flexible, because things like this can get in the way of cross-distro support. For example, this is currently some output from a Fedora machine:

$ hello-world.env | grep SNAP
SNAP_USER_COMMON=/home/<user>/snap/hello-world/common
SNAP_REEXEC=
SNAP_LIBRARY_PATH=/var/lib/snapd/lib/gl:
SNAP_COMMON=/var/snap/hello-world/common
SNAP_USER_DATA=/home/<user>/snap/hello-world/27
SNAP_DATA=/var/snap/hello-world/27
SNAP_REVISION=27
SNAP_NAME=hello-world
SNAP_ARCH=amd64
SNAP_VERSION=6.3
SNAP=/var/lib/snapd/snap/hello-world/27

Note how $SNAP is in a different place. This will hopefully change soon, but it's something to keep in mind.

[kyrofa]

I approve with the caveat that we should investigate making the client more flexible at runtime. Using absolute paths and an organize is a hack from which we should move away as soon as we can.

[3v1n0]

Ouch, I though this was somewhat decided that /snap/$SNAPCRAFT_PROJECT_NAME/current was always pointing to the snap current location... Is that because on fedora we've still an old snapd?

Keep in mind that most of desktop apps we snapped already use this assertion...

We can probably fix things in code, but it's annoying for now, as I think it needs some deeper changes.

[kyrofa]

Ouch, I though this was somewhat decided that /snap/$SNAPCRAFT_PROJECT_NAME/current was always pointing to the snap current location... Is that because on fedora we've still an old snapd?

An older snapd and an older snap-confine. Other distros are the same. Things trickle down, I'm afraid. I see code in the newest snap-confine that mentions dealing with this issue, but I have yet to see it in use.

Keep in mind that most of desktop apps we snapped already use this assertion...

Any time someone asked me this I said it was a bad idea for this very reason.

[enoch85]

Hope I wasn't too quick on the merge button...

[3v1n0]

I think this is fine for now, it's just an ubuntu-only solution, but we can improve in different branch.

[oparoz]

Could someone track @kyrofa's review comment in an issue so that we don't forget to address it as soon as we can?