Bug 1367825 - Review Request: snapd - The snapd and snap tools enable systems to work with .snap files
Summary: Review Request: snapd - The snapd and snap tools enable systems to work with ...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Neal Gompa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1360199
Blocks: 1367932
TreeView+ depends on / blocked
 
Reported: 2016-08-17 14:49 UTC by Zygmunt Krynicki
Modified: 2017-04-09 10:19 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2017-04-09 02:53:54 UTC
Type: ---
Embargoed:
ngompa13: fedora-review+


Attachments (Terms of Use)

Description Zygmunt Krynicki 2016-08-17 14:49:21 UTC
Spec URL: https://fedorapeople.org/~zyga/snapd.spec
SRPM URL: https://fedorapeople.org/~zyga/snapd-2.11-1.fc24.src.rpm
Description:
Snappy is a modern, cross-distribution, transactional package manager designed for
working with self-contained, immutable packages.

Fedora Account System Username: zyga

Comment 1 Neal Gompa 2016-08-17 14:51:35 UTC
Taking this review.

Comment 2 Neal Gompa 2016-08-17 15:05:08 UTC
A few things I see stand out:

* Presets are supposed to be centrally managed in Fedora. Please remove the preset from snapd and file a bug against fedora-release to add the snapd preset. This is per Fedora policy: https://fedoraproject.org/wiki/Packaging:DefaultServices

* If you require specific kernel modules, please use their kmod provides. For example, for squashfs, require "kmod(squashfs.ko)" instead of kernel-modules.

* Shouldn't modprobe be done before snapd is started? My understanding is that snapd itself triggers the mounts of snaps, so modprobe should probably occur as an ExecStartPre for snapd.

* I'm not sure it's a good idea to go and torch user data in the event snapd is uninstalled. Can snapd be (theoretically) uninstalled and then installed later and use the snaps/configs that already exist? If so, it *definitely* makes no sense to purge everything like that.

There may be more, but those stand out at the moment.

Comment 3 Zygmunt Krynicki 2016-08-17 16:44:01 UTC
(In reply to Neal Gompa from comment #2)
> A few things I see stand out:
> 
> * Presets are supposed to be centrally managed in Fedora. Please remove the
> preset from snapd and file a bug against fedora-release to add the snapd
> preset. This is per Fedora policy:
> https://fedoraproject.org/wiki/Packaging:DefaultServices

Understood, will do. I removed the presets now.
 
> * If you require specific kernel modules, please use their kmod provides.
> For example, for squashfs, require "kmod(squashfs.ko)" instead of
> kernel-modules.

All the nice things that Fedora has :-). Updated to use this.
 
> * Shouldn't modprobe be done before snapd is started? My understanding is
> that snapd itself triggers the mounts of snaps, so modprobe should probably
> occur as an ExecStartPre for snapd.

Ha, yes. This probably also explains why the Fedora cloud images didn't work with snapd (because squashfs wasn't available early enough). Let me know if the modification I did is okay.

> * I'm not sure it's a good idea to go and torch user data in the event snapd
> is uninstalled. Can snapd be (theoretically) uninstalled and then installed
> later and use the snaps/configs that already exist? If so, it *definitely*
> makes no sense to purge everything like that.

Yes it can be used as you described. Each distribution has different policy here, I'm totally happy to keep snapd state and state of particular snaps around (less scripts, less mistakes!).

Updated spec file at: https://fedorapeople.org/~zyga/snapd.spec

Comment 4 Zygmunt Krynicki 2016-08-17 16:47:44 UTC
The updated SRPM file is: https://fedorapeople.org/~zyga/snapd-2.11-5.fc24.src.rpm

Comment 5 Neal Gompa 2016-08-17 21:00:53 UTC
Two more things:

* Can you please tell me why you are pulling in /etc/environment in snapd.service through EnvironmentFile? The file is empty in Fedora. If there's a reason that a user may want to pass options or variables to snapd.service, then set this to "/etc/sysconfig/snapd" and create a file in that location. This file should be marked as %config(noreplace), since administrator changes should be preserved by default.

* Remove the portion of the %post scriptlet that starts the socket and timer. This is not allowed in Fedora without approval from FESCo. See https://fedoraproject.org/wiki/Packaging:DefaultServices for details. When the preset is pre-installed (as in fedora-release), then the unit is enabled and started automatically. Please file a bug using the following URL to get the presets in and to have the units started automatically in the event snapd is installed: https://bugzilla.redhat.com/enter_bug.cgi?product=Fedora&format=fedora-systemd-request

Comment 6 Zygmunt Krynicki 2016-08-17 21:07:01 UTC
(In reply to Neal Gompa from comment #5)
> Two more things:
> 
> * Can you please tell me why you are pulling in /etc/environment in
> snapd.service through EnvironmentFile? The file is empty in Fedora. If
> there's a reason that a user may want to pass options or variables to
> snapd.service, then set this to "/etc/sysconfig/snapd" and create a file in
> that location. This file should be marked as %config(noreplace), since
> administrator changes should be preserved by default.

This was done to teach snapd about proxy settings (http_proxy). If there's a better way to do that without having to synchronize proxy settings across applications please let me know


> 
> * Remove the portion of the %post scriptlet that starts the socket and
> timer. This is not allowed in Fedora without approval from FESCo. See
> https://fedoraproject.org/wiki/Packaging:DefaultServices for details. When
> the preset is pre-installed (as in fedora-release), then the unit is enabled
> and started automatically. Please file a bug using the following URL to get
> the presets in and to have the units started automatically in the event
> snapd is installed:
> https://bugzilla.redhat.com/enter_bug.cgi?product=Fedora&format=fedora-
> systemd-request

Ack. Will do!

Comment 7 Zygmunt Krynicki 2016-08-17 21:14:01 UTC
(In reply to Neal Gompa from comment #5)
> Two more things:
> 
> * Can you please tell me why you are pulling in /etc/environment in
> snapd.service through EnvironmentFile? The file is empty in Fedora. If
> there's a reason that a user may want to pass options or variables to
> snapd.service, then set this to "/etc/sysconfig/snapd" and create a file in
> that location. This file should be marked as %config(noreplace), since
> administrator changes should be preserved by default.
> 
> * Remove the portion of the %post scriptlet that starts the socket and
> timer. This is not allowed in Fedora without approval from FESCo. See

Done removing, working on the ticket now.

Updated spec file at: https://fedorapeople.org/~zyga/snapd.spec

Comment 8 Zygmunt Krynicki 2016-08-17 21:19:11 UTC
FESCO request: https://bugzilla.redhat.com/show_bug.cgi?id=1367932

Comment 10 Neal Gompa 2016-08-18 05:40:29 UTC
The usage of /snap hierarchy is forbidden in Fedora, see https://fedoraproject.org/wiki/Packaging:Guidelines#Filesystem_Layout

Would it be possible to relocate /snap to an FHS compliant location, like /var/lib/snapd/snaps? You're already using /var/lib/snapd/desktop for desktop files...

You also probably want /var/lib/snapd and associated subdirs to be owned by the snapd package. To do so, just add the following to the %files list:
%dir %{_sharedstatedir}/snapd
%dir %{_sharedstatedir}/snapd/desktop
%dir %{_sharedstatedir}/snapd/snaps

The above lines will ensure the directories exist and are owned by the snapd package.

Comment 11 Zygmunt Krynicki 2016-08-18 10:01:56 UTC
(In reply to Neal Gompa from comment #9)
> The URL used in Source0 is bad. See
> https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/
> SourceURL#Git_Tags
> 
> A suggestion: Set Source0 to
> "https://%{provider_prefix}/archive/%{version}/%{name}-%{version}.tar.gz"

This URL doesn't work.

This one does:

https://%{provider_prefix}/archive/%{version}.tar.gz

Comment 12 Zygmunt Krynicki 2016-08-18 10:11:46 UTC
(In reply to Neal Gompa from comment #10)
> The usage of /snap hierarchy is forbidden in Fedora, see
> https://fedoraproject.org/wiki/Packaging:Guidelines#Filesystem_Layout
> 
> Would it be possible to relocate /snap to an FHS compliant location, like
> /var/lib/snapd/snaps? You're already using /var/lib/snapd/desktop for
> desktop files...

I'm not sure yet. I'm exploring options at this time.

> You also probably want /var/lib/snapd and associated subdirs to be owned by
> the snapd package. To do so, just add the following to the %files list:
> %dir %{_sharedstatedir}/snapd
> %dir %{_sharedstatedir}/snapd/desktop
> %dir %{_sharedstatedir}/snapd/snaps

Done in -8, along with the, apparently now working, Source0 URL

Pushed to the same address and to github.

Thanks
ZK

Comment 13 Neal Gompa 2016-08-18 10:14:17 UTC
That part looks good.

I just noticed that your License tag is invalid. "GPL-3" needs to be replaced with "GPLv3". If it's v3 or any later version, set it to "GPLv3+".

Comment 14 Zygmunt Krynicki 2016-08-18 10:49:16 UTC
I've updated snapd to 2.12 without any packaging changes.

Comment 15 Zygmunt Krynicki 2016-08-18 10:52:12 UTC
(In reply to Neal Gompa from comment #13)
> That part looks good.
> 
> I just noticed that your License tag is invalid. "GPL-3" needs to be
> replaced with "GPLv3". If it's v3 or any later version, set it to "GPLv3+".

Updated in 2.12-2 now, thanks for spotting this!

Comment 16 Neal Gompa 2016-08-18 13:09:02 UTC
You should fix the changelog versions for the two newest entries. They say 2.11 when they should say 2.12.

Comment 17 Zygmunt Krynicki 2016-08-18 13:31:32 UTC
(In reply to Neal Gompa from comment #16)
> You should fix the changelog versions for the two newest entries. They say
> 2.11 when they should say 2.12.

Fixed and pushed

Comment 18 Neal Gompa 2016-08-18 17:55:11 UTC
An exception for /snap has been requested to FESCo: https://fedorahosted.org/fesco/ticket/1614

Please prepare a contingency plan (such as an alternate path in /var/lib/snapd/, /run/snapd/, etc.) in the event it is rejected, otherwise this package cannot be added to Fedora.

Comment 19 Zbigniew Jędrzejewski-Szmek 2016-08-19 03:56:40 UTC
The summary of "the snapd and snap tools enable systems to work with .snap files" is not very informative: it should say what the snapd daemon actually does. (It's like saying that gcc allows programmers to work with .c files).

In the built binary rpm:
#/etc/profile.d/snapd.sh

PATH=/home/zbyszek/bin:/usr/lib64/qt-3.3/bin:/usr/lib64/ccache:/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/snap/bin
if [ -z "" ]; then
    XDG_DATA_DIRS=/usr/local/share/:/usr/share/:/var/lib/snapd/desktop
else
    XDG_DATA_DIRS="":/var/lib/snapd/desktop
fi
export XDG_DATA_DIRS

This doesn't look right ;)

/usr/lib/systemd/system-preset/91-snapd.preset → no, that's what the FESCo ticket is for.

Comment 20 Zbigniew Jędrzejewski-Szmek 2016-10-25 16:03:25 UTC
https://pagure.io/fesco/issue/1614#comment-27728

> We rejected this exception during the last meeting:
> * REJECTED: FHS exception for /snap (9:+ 0:- 0:0)

So, what the is the plan going forward here?

Comment 21 Neal Gompa 2016-10-25 19:47:54 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #20)
> https://pagure.io/fesco/issue/1614#comment-27728
> 
> > We rejected this exception during the last meeting:
> > * REJECTED: FHS exception for /snap (9:+ 0:- 0:0)
> 
> So, what the is the plan going forward here?

I met with Zygmunt at last weeks Snappy developer sprint and we've agreed to relocate /snap to /var/lib/snapd/snap.

snap-confine has already been updated to use this new path[1].

[1]: http://pkgs.fedoraproject.org/cgit/rpms/snap-confine.git/tree/snap-confine.spec#n41

Zygmunt will soon update this ticket with an updated snapd spec.

Comment 22 Zygmunt Krynicki 2016-11-01 13:43:26 UTC
Updated spec and SRPM

Spec URL: https://fedorapeople.org/~zyga/snapd.spec
SRPM URL: https://fedorapeople.org/~zyga/snapd-2.16-1.fc24.src.rpm

Comment 23 Neal Gompa 2016-11-01 13:48:10 UTC
Package was generated through gofed, simplifying the review considerably.

- Conforms to packaging guidelines (gofed generated spec)
- license correct and valid
- Binaries for applications installed, sources installed for devel subpackage
- SELinux module subpackage uses appropriate macros
- SELinux module subpackage is appropriately required by main package

PACKAGE APPROVED

Comment 24 Gwyn Ciesla 2016-11-01 14:17:05 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/snapd

Comment 25 Fedora Update System 2017-03-31 19:35:02 UTC
snapd-2.23.6-1.fc26 snapd-glib-1.9-2.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-261aa8c9f4

Comment 26 Fedora Update System 2017-03-31 19:37:34 UTC
snapd-glib-1.9-2.fc25 snapd-2.23.6-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-37a7331620

Comment 27 Fedora Update System 2017-03-31 19:40:19 UTC
snapd-glib-1.9-2.fc24 snapd-2.23.6-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-ce0fdd87a4

Comment 28 Fedora Update System 2017-04-01 23:22:34 UTC
snapd-2.23.6-2.fc24, snapd-glib-1.9-2.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-ce0fdd87a4

Comment 29 Fedora Update System 2017-04-01 23:25:18 UTC
snapd-2.23.6-2.fc25, snapd-glib-1.9-2.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-37a7331620

Comment 30 Fedora Update System 2017-04-02 00:52:15 UTC
snapd-2.23.6-2.fc26, snapd-glib-1.9-2.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-261aa8c9f4

Comment 31 Fedora Update System 2017-04-02 00:56:13 UTC
snapd-2.23.6-3.fc26 snapd-glib-1.9-2.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-261aa8c9f4

Comment 32 Fedora Update System 2017-04-02 00:58:05 UTC
snapd-2.23.6-3.fc24 snapd-glib-1.9-2.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-ce0fdd87a4

Comment 33 Fedora Update System 2017-04-02 00:58:46 UTC
snapd-2.23.6-3.fc25 snapd-glib-1.9-2.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-37a7331620

Comment 34 Fedora Update System 2017-04-03 02:20:53 UTC
snapd-2.23.6-3.fc24, snapd-glib-1.9-2.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-ce0fdd87a4

Comment 35 Fedora Update System 2017-04-03 02:20:56 UTC
snapd-2.23.6-3.fc25, snapd-glib-1.9-2.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-37a7331620

Comment 36 Fedora Update System 2017-04-03 03:51:49 UTC
snapd-2.23.6-3.fc26, snapd-glib-1.9-2.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-261aa8c9f4

Comment 37 Fedora Update System 2017-04-05 13:38:07 UTC
snapd-2.23.6-4.fc26 snapd-glib-1.10-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-261aa8c9f4

Comment 38 Fedora Update System 2017-04-05 13:39:20 UTC
snapd-2.23.6-4.fc25 snapd-glib-1.10-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-37a7331620

Comment 39 Fedora Update System 2017-04-05 13:40:43 UTC
snapd-2.23.6-3.fc24 snapd-glib-1.10-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-ce0fdd87a4

Comment 40 Fedora Update System 2017-04-05 19:53:39 UTC
snapd-2.23.6-4.fc25, snapd-glib-1.10-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-37a7331620

Comment 41 Fedora Update System 2017-04-05 21:53:52 UTC
snapd-2.23.6-4.fc26, snapd-glib-1.10-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-261aa8c9f4

Comment 42 Fedora Update System 2017-04-09 02:53:54 UTC
snapd-2.23.6-4.fc25, snapd-glib-1.10-1.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 43 Fedora Update System 2017-04-09 05:58:34 UTC
snapd-2.23.6-4.fc26, snapd-glib-1.10-1.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

Comment 44 Fedora Update System 2017-04-09 10:19:50 UTC
snapd-2.23.6-3.fc24, snapd-glib-1.10-1.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.